Skip to content

Commit

Permalink
go1.5: fixed tests of time package
Browse files Browse the repository at this point in the history
  • Loading branch information
neelance committed Aug 2, 2015
1 parent d13c606 commit f2c554a
Showing 1 changed file with 14 additions and 2 deletions.
16 changes: 14 additions & 2 deletions compiler/natives/time/time.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
package time

import (
"errors"
"runtime"
"strings"

"github.com/gopherjs/gopherjs/js"
Expand Down Expand Up @@ -74,5 +74,17 @@ func stopTimer(t *runtimeTimer) bool {
}

func loadLocation(name string) (*Location, error) {
return nil, errors.New("unknown time zone " + name)
return loadZoneFile(runtime.GOROOT()+"/lib/time/zoneinfo.zip", name)

This comment has been minimized.

Copy link
@dmitshur

dmitshur Aug 3, 2015

Member

I think this should be:

loadZoneFile(filepath.Join(runtime.GOROOT(), "lib", "time", "zoneinfo.zip"), name)

Or equivalent that doesn't use filepath package, if importing it inside time package is too costly.

That might be the case. Go 1.5 standard time package doesn't import filepath (although it imports syscall, runtime, etc.), it just implements loadLocation differently on each platform (windows, unix, ios, etc.).

But if this is only needed for tests, then I think importing filepath (for tests only) is completely justified, since performance for tests is not as important as normal code.

If it really makes sense not to use proper filepath manipulation here, then at the very least there should be a comment explaining why incorrect behavior is being used. Otherwise this code screams "this is wrong, this is a bug" for me when I read it.

}

func forceZipFileForTesting(zipOnly bool) {
}

func initTestingZone() {
z, err := loadLocation("America/Los_Angeles")
if err != nil {
panic("cannot load America/Los_Angeles for testing: " + err.Error())
}
z.name = "Local"
localLoc = *z
}

0 comments on commit f2c554a

Please sign in to comment.