Repository: rclone/rclone
Pull request #4573 (2020-09-07 01:53:35)

Fix test failure in different local time zones
<!--
Thank you very much for contributing code or documentation to rclone! Please
fill out the following questions to make it easier for us to review your
changes.

You do not need to check all the boxes below all at once, feel free to take
your time and add more commits. If you're done and ready for review, please
check the last box.
-->

#### What is the purpose of this change?
To fix a broken unit test.

TestParseDuration relied on an elapsed time calculation which
would vary based on the system local time. Fix the test by not relying
on the system time location. Also make the test more deterministic
by injecting time in tests rather than using system time.



#### Was the change discussed in an issue or in the forum before?

#4529

#### Checklist

- [x] I have read the [contribution guidelines](https://github.com/rclone/rclone/blob/master/CONTRIBUTING.md#submitting-a-pull-request).
- [x] I have added tests for all changes in this PR if appropriate.
- [x] I have added documentation for the changes if appropriate.
- [x] All commit messages are in [house style](https://github.com/rclone/rclone/blob/master/CONTRIBUTING.md#commit-messages).
- [x] I'm done, this Pull Request is ready for review :-)


ivandeex: @Zacho2 Are you still with us?

> ParseDuration is supposed to mimic the `time.ParseDuration` interface so adding another parameter to it isn't ideal.
> I guess we could have a `var parseDurationTimeNow = time.Now` and monkey patch that in the tests?
> Or make a `parseDuration` which takes the extra parameter and test that.


Zacho2: @ncw imo the `time` package doesn't export an interface for `time.ParseDuration` so we don't need to follow the function signature it to a T. Monkey`ing or a using a package global seems less than ideal to me. My patch minimizes `time.Now()` calls by only calling when the duration can't be parsed by the `time` package.

I could create a `parseDuration` function in package `fs`, but it seems like it would have to call `time.Now()` for all duration strings. Is this a concern?

ncw: >  imo the `time` package doesn't export an interface for `time.ParseDuration` so we don't need to follow the function signature it to a T.

I would prefer not to add an extra parameter to `fs.ParseDuration` as it is supposed to be a drop in replacement for `time.ParseDuration`.

> Monkey patching or a using a package global seems less than ideal to me. My patch minimizes `time.Now()` calls by only calling when the duration can't be parsed by the `time` package.

Noted

> I could create a `parseDuration` function in package `fs`, but it seems like it would have to call `time.Now()` for all duration strings. Is this a concern?

How about creating a `ParseDurationFromNow` (say) which takes the `func() time.Time` pointer and we make a simple wrapper for `ParseDuration` so we don't have to change its interface. The tests can test `ParseDurationFromNow`. This keeps the efficiency of not calling time.Time where necessary and keeps the interface of `ParseDuration` the same.

Zacho2: Thanks @ncw. I've implemented your suggestion.

Patch:
diff --git a/backend/filefabric/filefabric.go b/backend/filefabric/filefabric.go
index fb6241b04..4307f5b21 100644
--- a/backend/filefabric/filefabric.go
+++ b/backend/filefabric/filefabric.go
@@ -536,7 +536,7 @@ func (f *Fs) NewObject(ctx context.Context, remote string) (fs.Object, error) {
 func (f *Fs) FindLeaf(ctx context.Context, pathID, leaf string) (pathIDOut string, found bool, err error) {
 	// Find the leaf in pathID
 	found, err = f.listAll(ctx, pathID, true, false, func(item *api.Item) bool {
-		if item.Name == leaf {
+		if strings.EqualFold(item.Name, leaf) {
 			pathIDOut = item.ID
 			return true
 		}
diff --git a/backend/opendrive/opendrive.go b/backend/opendrive/opendrive.go
index e58ad5781..43352cde0 100644
--- a/backend/opendrive/opendrive.go
+++ b/backend/opendrive/opendrive.go
@@ -721,7 +721,7 @@ func (f *Fs) FindLeaf(ctx context.Context, pathID, leaf string) (pathIDOut strin
 	for _, folder := range folderList.Folders {
 		// fs.Debugf(nil, "Folder: %s (%s)", folder.Name, folder.FolderID)
 
-		if leaf == folder.Name {
+		if strings.EqualFold(leaf, folder.Name) {
 			// found
 			return folder.FolderID, true, nil
 		}
diff --git a/backend/premiumizeme/premiumizeme.go b/backend/premiumizeme/premiumizeme.go
index a329102ca..eca5cd7f7 100644
--- a/backend/premiumizeme/premiumizeme.go
+++ b/backend/premiumizeme/premiumizeme.go
@@ -345,7 +345,7 @@ func (f *Fs) NewObject(ctx context.Context, remote string) (fs.Object, error) {
 func (f *Fs) FindLeaf(ctx context.Context, pathID, leaf string) (pathIDOut string, found bool, err error) {
 	// Find the leaf in pathID
 	found, err = f.listAll(ctx, pathID, true, false, func(item *api.Item) bool {
-		if item.Name == leaf {
+		if strings.EqualFold(item.Name, leaf) {
 			pathIDOut = item.ID
 			return true
 		}
diff --git a/backend/s3/v2sign.go b/backend/s3/v2sign.go
index 2c22378e3..d31cbeb71 100644
--- a/backend/s3/v2sign.go
+++ b/backend/s3/v2sign.go
@@ -53,6 +53,7 @@ func sign(AccessKey, SecretKey string, req *http.Request) {
 	var md5 string
 	var contentType string
 	var headersToSign []string
+	tmpHeadersToSign := make(map[string][]string)
 	for k, v := range req.Header {
 		k = strings.ToLower(k)
 		switch k {
@@ -62,15 +63,24 @@ func sign(AccessKey, SecretKey string, req *http.Request) {
 			contentType = v[0]
 		default:
 			if strings.HasPrefix(k, "x-amz-") {
-				vall := strings.Join(v, ",")
-				headersToSign = append(headersToSign, k+":"+vall)
+				tmpHeadersToSign[k] = v
 			}
 		}
 	}
+	var keys []string
+	for k := range tmpHeadersToSign {
+		keys = append(keys, k)
+	}
+	// https://docs.aws.amazon.com/AmazonS3/latest/dev/RESTAuthentication.html
+	sort.Strings(keys)
+
+	for _, key := range keys {
+		vall := strings.Join(tmpHeadersToSign[key], ",")
+		headersToSign = append(headersToSign, key+":"+vall)
+	}
 	// Make headers of interest into canonical string
 	var joinedHeadersToSign string
 	if len(headersToSign) > 0 {
-		sort.StringSlice(headersToSign).Sort()
 		joinedHeadersToSign = strings.Join(headersToSign, "\n") + "\n"
 	}
 
diff --git a/docs/content/commands/rclone_serve_ftp.md b/docs/content/commands/rclone_serve_ftp.md
index fadbf2b9f..204e7eb71 100644
--- a/docs/content/commands/rclone_serve_ftp.md
+++ b/docs/content/commands/rclone_serve_ftp.md
@@ -277,7 +277,7 @@ together, if `--auth-proxy` is set the authorized keys option will be
 ignored.
 
 There is an example program
-[bin/test_proxy.py](https://github.com/rclone/rclone/blob/master/test_proxy.py)
+[bin/test_proxy.py](https://github.com/rclone/rclone/blob/master/bin/test_proxy.py)
 in the rclone source code.
 
 The program's job is to take a `user` and `pass` on the input and turn
diff --git a/docs/content/commands/rclone_serve_sftp.md b/docs/content/commands/rclone_serve_sftp.md
index 3f750f9f7..b4deb13f4 100644
--- a/docs/content/commands/rclone_serve_sftp.md
+++ b/docs/content/commands/rclone_serve_sftp.md
@@ -288,7 +288,7 @@ together, if `--auth-proxy` is set the authorized keys option will be
 ignored.
 
 There is an example program
-[bin/test_proxy.py](https://github.com/rclone/rclone/blob/master/test_proxy.py)
+[bin/test_proxy.py](https://github.com/rclone/rclone/blob/master/bin/test_proxy.py)
 in the rclone source code.
 
 The program's job is to take a `user` and `pass` on the input and turn
diff --git a/fs/parseduration.go b/fs/parseduration.go
index cd3f192ac..b5104cc6c 100644
--- a/fs/parseduration.go
+++ b/fs/parseduration.go
@@ -88,8 +88,9 @@ func parseDurationDates(age string, epoch time.Time) (t time.Duration, err error
 	return t, err
 }
 
-// ParseDuration parses a duration string. Accept ms|s|m|h|d|w|M|y suffixes. Defaults to second if not provided
-func ParseDuration(age string) (d time.Duration, err error) {
+// parseDurationFromNow parses a duration string. Allows ParseDuration to match the time
+// package and easier testing within the fs package.
+func parseDurationFromNow(age string, getNow func() time.Time) (d time.Duration, err error) {
 	if age == "off" {
 		return time.Duration(DurationOff), nil
 	}
@@ -105,7 +106,7 @@ func ParseDuration(age string) (d time.Duration, err error) {
 		return d, nil
 	}
 
-	d, err = parseDurationDates(age, time.Now())
+	d, err = parseDurationDates(age, getNow())
 	if err == nil {
 		return d, nil
 	}
@@ -113,6 +114,11 @@ func ParseDuration(age string) (d time.Duration, err error) {
 	return d, err
 }
 
+// ParseDuration parses a duration string. Accept ms|s|m|h|d|w|M|y suffixes. Defaults to second if not provided
+func ParseDuration(age string) (time.Duration, error) {
+	return parseDurationFromNow(age, time.Now)
+}
+
 // ReadableString parses d into a human readable duration.
 // Based on https://github.com/hako/durafmt
 func (d Duration) ReadableString() string {


Test patch:
diff --git a/fs/parseduration_test.go b/fs/parseduration_test.go
index 04830ef06..69de520ca 100644
--- a/fs/parseduration_test.go
+++ b/fs/parseduration_test.go
@@ -15,6 +15,11 @@ import (
 var _ pflag.Value = (*Duration)(nil)
 
 func TestParseDuration(t *testing.T) {
+	now := time.Date(2020, 9, 5, 8, 15, 5, 250, time.UTC)
+	getNow := func() time.Time {
+		return now
+	}
+
 	for _, test := range []struct {
 		in   string
 		want time.Duration
@@ -37,12 +42,12 @@ func TestParseDuration(t *testing.T) {
 		{"1x", 0, true},
 		{"off", time.Duration(DurationOff), false},
 		{"1h2m3s", time.Hour + 2*time.Minute + 3*time.Second, false},
-		{"2001-02-03", time.Since(time.Date(2001, 2, 3, 0, 0, 0, 0, time.UTC)), false},
-		{"2001-02-03 10:11:12", time.Since(time.Date(2001, 2, 3, 10, 11, 12, 0, time.UTC)), false},
-		{"2001-02-03T10:11:12", time.Since(time.Date(2001, 2, 3, 10, 11, 12, 0, time.UTC)), false},
-		{"2001-02-03T10:11:12.123Z", time.Since(time.Date(2001, 2, 3, 10, 11, 12, 123, time.UTC)), false},
+		{"2001-02-03", now.Sub(time.Date(2001, 2, 3, 0, 0, 0, 0, time.UTC)), false},
+		{"2001-02-03 10:11:12", now.Sub(time.Date(2001, 2, 3, 10, 11, 12, 0, time.UTC)), false},
+		{"2001-02-03T10:11:12", now.Sub(time.Date(2001, 2, 3, 10, 11, 12, 0, time.UTC)), false},
+		{"2001-02-03T10:11:12.123Z", now.Sub(time.Date(2001, 2, 3, 10, 11, 12, 123, time.UTC)), false},
 	} {
-		duration, err := ParseDuration(test.in)
+		duration, err := parseDurationFromNow(test.in, getNow)
 		if test.err {
 			require.Error(t, err)
 		} else {
@@ -58,6 +63,11 @@ func TestParseDuration(t *testing.T) {
 }
 
 func TestDurationString(t *testing.T) {
+	now := time.Date(2020, 9, 5, 8, 15, 5, 250, time.UTC)
+	getNow := func() time.Time {
+		return now
+	}
+
 	for _, test := range []struct {
 		in   time.Duration
 		want string
@@ -88,7 +98,7 @@ func TestDurationString(t *testing.T) {
 		got := Duration(test.in).String()
 		assert.Equal(t, test.want, got)
 		// Test the reverse
-		reverse, err := ParseDuration(test.want)
+		reverse, err := parseDurationFromNow(test.want, getNow)
 		assert.NoError(t, err)
 		assert.Equal(t, test.in, reverse)
 	}
diff --git a/fstest/fstests/fstests.go b/fstest/fstests/fstests.go
index b88559f70..672c2a55b 100644
--- a/fstest/fstests/fstests.go
+++ b/fstest/fstests/fstests.go
@@ -307,6 +307,17 @@ func stringsContains(x string, ss []string) bool {
 	return false
 }
 
+// toUpperASCII returns a copy of the string s with all Unicode
+// letters mapped to their upper case.
+func toUpperASCII(s string) string {
+	return strings.Map(func(r rune) rune {
+		if 'a' <= r && r <= 'z' {
+			r -= 'a' - 'A'
+		}
+		return r
+	}, s)
+}
+
 // Run runs the basic integration tests for a remote using the options passed in.
 //
 // They are structured in a hierarchical way so that dependencies for the tests can be created.
@@ -1008,10 +1019,12 @@ func Run(t *testing.T, opt *Opt) {
 				if !f.Features().CaseInsensitive {
 					t.Skip("Not Case Insensitive")
 				}
-				obj := findObject(ctx, t, f, strings.ToUpper(file1.Path))
+				obj := findObject(ctx, t, f, toUpperASCII(file1.Path))
 				file1.Check(t, obj, f.Precision())
-				obj = findObject(ctx, t, f, strings.ToUpper(file2.Path))
-				file2.Check(t, obj, f.Precision())
+				t.Run("Dir", func(t *testing.T) {
+					obj := findObject(ctx, t, f, toUpperASCII(file2.Path))
+					file2.Check(t, obj, f.Precision())
+				})
 			})
 
 			// TestFsListFile1and2 tests two files present