internal/syscall/windows/registry: TestGetMUIStringValue fails #13502
Comments
ianlancetaylor
changed the title
|
@mattn, what version of Windows? |
rsc
changed the title
|
@mattn what does your TestGetMUIStringValue output look like? Thank you. Alex |
|
Windows10 64bit @alexbrainman here is: |
|
Like mattn says, the test (my test gulp) did not take into account the value of DynamicDaylightTimeDisabled, it assumes (incorrectly) that the value is always FALSE. When dtzi.DynamicDaylightTimeDisabled=0 then dtzi.StandardName != dtzi.DaylightName and Mui_Std=dtzi.StandardName and Mui_Dlt=dtzi.DaylightName When dtzi.DynamicDaylightTimeDisabled=1 then dtzi.StandardName=dtzi.DaylightName and Mui_Std=dtzi.StandardName (Mui_Dlt can not be checked in this case through dzti) |
|
This test was never the best solution in my mind, it was a compromise between building a golden resource file to test RegLoadMUIString (which had it's own problems with registry caching of key values) and testing the real keys (MUI_xxx) that actually uses tzres.dll ... Anyway, we could fix this test for the case when dtzi.DynamicDaylightTimeDisabled=1 (still works for MUI_Std), disable the test altogether (RegLoadMUIString is still tested through TestToEnglishName), or a third option? thoughts? |
|
@dajoo75 lets fix the test. Would you like to send a CL? Thank you. Alex |
|
No problem, I'll send a CL. |
|
@dajoo75, did you send a CL? |
|
No, still working on it. You can still choose to remove the test if it needs to be fixed asap.
|
|
CL https://golang.org/cl/17998 mentions this issue. |
|
Hi! Sorry I was unable to send a CL in time. Would you be interested in another solution if I send it at a later date? (Not making any promises this time ;)). The current solution was/is for me a Hail Mary with severe limitations and I don't know what will happen if a user has called SetDynamicTimeZoneInformation directly or indirectly or if there are other unknowns since this test depends on the state of the user's computer which we know nothing about. I've been scouring the internet for information about a way to test this function using known values. The best I can come up with is injecting the Windows Shell String Cache with known strings. This, together with mocking regLoadMUIString for testing e.g. error values, panics and the buffer growing would allow for better testing of this function. However, all of the above could still be way to much effort for testing something that seems relatively stable and does not involve much code and is also somewhat tested indirectly through TestToEnglishName... |
|
I don't think we should spent more time on this test. My fix is good enough. Alex |
|
Ok!
|
|
FYI. In case anyone is interested/would benefit: I got the shell string cache solution mentioned above working (example code below) but I would no longer recommend that solution. The system still requires a resource dll (and you will probably need two versions, one for 64-bit systems and one for 32-bit as well; I have no way of testing the latter). The system also requires that the resource IDs exists in the dll. Once you have those requirements down you can inject the shell string cache with known values and those will be returned when you call GetMUIStringValue. The string cache is however located in different locations depending on OS version. All in all this feels very brittle and complex, so like I said, I would not recommend this solution if the current strategy (testing GetMUIStringValue against another Windows function) ever needs to be revised. Example code (could contain bugs): func TestGetMUIStringValue(t *testing.T) {
wd, _ := os.Getwd()
var tests = []struct {
name string
want interface{}
data string
cache bool // if true, inject data into shell string cache, otherwise false
}{
{name: "Valid string (with path)", want: "a value", data: "@" + wd + "\\testdata\\resource.dll,-666", cache: true},
{name: "Invalid string (wrong format)", want: error(registry.ErrInvalidData), data: "a value"},
{name: "Invalid string (dll missing)", want: error(registry.ErrNotExist), data: "@shouldnotexist666.dll,-1"},
}
if err := registry.LoadRegLoadMUIString(); err != nil {
t.Skip("regLoadMUIString not supported; skipping")
}
basek, kpath := registry.CURRENT_USER, "Software\\"+randKeyName("TestMUIValues_")
k, err := tempSubKey(basek, kpath)
if err != nil {
t.Fatal(err)
}
defer k.Close()
defer registry.DeleteKey(basek, kpath)
for _, test := range tests {
if err = k.SetStringValue(test.name, test.data); err != nil {
t.Fatalf("SetValue for %q failed: %v", test.name, err)
}
}
defer walkShellStringCache(func(k registry.Key) {
for _, test := range tests {
if test.cache {
k.DeleteValue(test.data)
}
}
})
walkShellStringCache(func(k registry.Key) {
for _, test := range tests {
if test.cache {
if err = k.SetStringValue(test.data, test.want.(string)); err != nil {
t.Fatalf("SetValue for %q failed: %v", test.name, err)
}
}
}
})
for _, test := range tests {
var got interface{}
got, err = k.GetMUIStringValue(test.name)
if _, ok := test.want.(error); ok {
got = err
}
if got != test.want {
t.Errorf("GetMUIStringValue: %s: Got %T %q, want %T %q", test.name, got, got, test.want, test.want)
}
}
}
func tempSubKey(basek registry.Key, path string) (k registry.Key, err error) {
k, _, err = registry.CreateKey(basek, path, registry.CREATE_SUB_KEY|registry.QUERY_VALUE|registry.SET_VALUE)
if err != nil {
return 0, err
}
return k, nil
}
func walkShellStringCache(visitorFn func(registry.Key)) error {
keys := []struct {
k registry.Key
path string
}{
// https://msdn.microsoft.com/en-us/goglobal/bb688098.aspx
//{registry.CURRENT_USER, `SOFTWARE\Classes\Local Settings\RegMuiCache`}, // Vista and Windows 2008
//{registry.CURRENT_USER, `SOFTWARE\Classes\Local Settings\Software\Microsoft\Windows\Shell\MuiCache`}, // Vista and Windows 2008
{registry.CURRENT_USER, `SOFTWARE\Classes\Local Settings\MuiCache`}, // Windows 7
//{registry.CLASSES_ROOT, `Local Settings\MuiCache`}, // Windows 7
}
for _, key := range keys {
if err := walkLeafs(key.k, key.path, visitorFn); err != nil && err != registry.ErrNotExist {
return err
}
}
return nil
}
func walkLeafs(k registry.Key, path string, visitorFn func(registry.Key)) error {
subk, err := registry.OpenKey(k, path, registry.ENUMERATE_SUB_KEYS|registry.QUERY_VALUE|registry.SET_VALUE)
if err != nil {
if err == syscall.ERROR_ACCESS_DENIED {
// ignore error, if we are not allowed to access this key
return nil
}
return err
}
defer subk.Close()
subkNames, err := subk.ReadSubKeyNames(-1)
if err != nil {
return err
}
if len(subkNames) == 0 {
visitorFn(subk)
return nil
}
for _, subkName := range subkNames {
if err := walkLeafs(k, path+"\\"+subkName, visitorFn); err != nil {
return err
}
}
return nil
} |
|
I think we'll stick with my change instead. :-) Alex |
|
SGTM :) |
|
A final note on this (hopefully): SHLoadIndirectString is an alternative to GetDynamicTimeZoneInformation that you can remember if the current code needs to be revised at some later date. I think SHLoadIndirectString is better suited for the current testing approach since the caller get to decide what registry key values to fetch irrespective of any settings the user has made. I still think that an integration test is the right approach (and not a pure unit test with mocking) and also testing the real MUI-keys and indirectly tzres.dll and its satellite DLLs since that was the initial purpose for GetMUIStringValue. Testing the real keys will also constrict the side effects (populating the shell string cache) to keys that Windows itself uses which I feel is OK. Example to put in your back pocket for a rainy day (could contain bugs): //Solution using SHLoadIndirectString
//
// $ cd %goroot%\src\internal\syscall\windows\registry
// $ go test -run=TestGetMUIStringValue -cover -coverprofile=cover.out
// $ go tool cover -html cover.out
func TestGetMUIStringValue(t *testing.T) {
if err := registry.LoadRegLoadMUIString(); err != nil {
t.Skip("regLoadMUIString not supported; skipping")
}
tzk := openTZKey("W. Europe Standard Time")
defer tzk.close()
tests := []struct {
name string
want interface{}
data string
}{
{"MUI_Std", tzk.load("MUI_Std"), tzk.data("MUI_Std")},
{"MUI_Dlt", tzk.load("MUI_Dlt"), tzk.data("MUI_Dlt")},
{"MUI_Display", tzk.load("MUI_Display"), tzk.data("MUI_Display")},
//{"Invalid string (wrong format)", error(registry.ErrInvalidData), "a value"},
{"Invalid string (dll missing)", error(registry.ErrNotExist), "@shouldnotexist666.dll,-1"},
}
if tzk.err != nil {
t.Fatal(tzk.err)
}
basek, kpath := registry.CURRENT_USER, "Software\\"+randKeyName("TestMUIValues_")
k, err := tempSubKey(basek, kpath)
if err != nil {
log.Fatal(err)
}
defer k.Close()
defer registry.DeleteKey(basek, kpath)
for _, test := range tests {
if err = k.SetStringValue(test.name, test.data); err != nil {
t.Fatalf("SetValue for %q failed: %v", test.name, err)
}
}
//*registry.PGetMuiStringValueBufLen = 2
for _, test := range tests {
var got interface{}
got, err = k.GetMUIStringValue(test.name)
if _, ok := test.want.(error); ok {
got = err
}
if got != test.want {
t.Errorf("GetMUIStringValue: %s: Got %T %q, want %T %q", test.name, got, got, test.want, test.want)
}
}
}
type TZKey struct {
k registry.Key
err error
}
func (tzk *TZKey) close() {
if tzk.k != 0 {
tzk.k.Close()
}
}
func (tzk *TZKey) data(name string) string {
if tzk.err != nil {
return ""
}
var v string
v, _, tzk.err = tzk.k.GetStringValue(name)
if tzk.err != nil {
return ""
}
return v
}
func (tzk *TZKey) load(name string) string {
v := tzk.data(name)
if tzk.err != nil {
return ""
}
var s string
s, tzk.err = SHLoadIndirectString(v)
if tzk.err != nil {
return ""
}
return s
}
func openTZKey(tz string) *TZKey {
tzk := new(TZKey)
tzk.k, tzk.err = registry.OpenKey(registry.LOCAL_MACHINE, `SOFTWARE\Microsoft\Windows NT\CurrentVersion\Time Zones\`+tz, registry.QUERY_VALUE)
return tzk
}
func tempSubKey(basek registry.Key, path string) (k registry.Key, err error) {
k, _, err = registry.CreateKey(basek, path, registry.CREATE_SUB_KEY|registry.QUERY_VALUE|registry.SET_VALUE)
if err != nil {
return 0, err
}
return k, nil
}
var (
shlwapiDLL = syscall.NewLazyDLL("shlwapi")
procSHLoadIndirectString = shlwapiDLL.NewProc("SHLoadIndirectString")
)
func SHLoadIndirectString(src string) (res string, err error) {
psrc, err := syscall.UTF16PtrFromString(src)
if err != nil {
return "", err
}
var buf = make([]uint16, 1024)
r0, _, e1 := syscall.Syscall6(
procSHLoadIndirectString.Addr(),
4,
uintptr(unsafe.Pointer(psrc)),
uintptr(unsafe.Pointer(&buf[0])),
uintptr(len(buf)),
0,
0,
0)
if r0 != 0 {
return "", e1
}
return syscall.UTF16ToString(buf), nil
} |
The information getting from GetDynamicTimeZoneInformation is not same as
SOFTWARE\Microsoft\Windows NT\CurrentVersion\Time Zones\. Because DayLightString value onSOFTWARE\Microsoft\Windows NT\CurrentVersion\Time Zones\contains summer time sentences. For example, on my windows,Dlt is
東京 (夏時間)(mean Tokyo(SummerTime)). But DaylightName is東京 (標準時)(mean Tokyo(StandardTime)). So the test will be passed on the country where have summer-time.The test should be disabled when dtzi.DynamicDaylightTimeDisabled is TRUE. Or remove.
/cc @alexbrainman
The text was updated successfully, but these errors were encountered: