Misc fixes and updated CI (#514)

* Misc fixes and updated CI

Fix race condition when refreshing token.
Fixed some tests to work with Go 1.14.
Updated CI to run latest two versions of Go.

* fix job name

* more test clean-up

CHANGELOG.md

@@ -1,5 +1,12 @@
 # CHANGELOG
 
+## v14.0.1
+
+### Bug Fixes
+
+- Fix race condition when refreshing token.
+- Fixed some tests to work with Go 1.14.
+
 ## v14.0.0
 
 ## Breaking Changes

autorest/adal/token.go

@@ -771,8 +771,9 @@ func (spt *ServicePrincipalToken) EnsureFresh() error {
 // EnsureFreshWithContext will refresh the token if it will expire within the refresh window (as set by
 // RefreshWithin) and autoRefresh flag is on.  This method is safe for concurrent use.
 func (spt *ServicePrincipalToken) EnsureFreshWithContext(ctx context.Context) error {
-	if spt.inner.AutoRefresh && spt.inner.Token.WillExpireIn(spt.inner.RefreshWithin) {
-		// take the write lock then check to see if the token was already refreshed
+	// must take the read lock when initially checking the token's expiration
+	if spt.inner.AutoRefresh && spt.Token().WillExpireIn(spt.inner.RefreshWithin) {
+		// take the write lock then check again to see if the token was already refreshed
 		spt.refreshLock.Lock()
 		defer spt.refreshLock.Unlock()
 		if spt.inner.Token.WillExpireIn(spt.inner.RefreshWithin) {

autorest/adal/token_test.go

@@ -164,7 +164,7 @@ func TestServicePrincipalTokenRefreshUsesCustomRefreshFunc(t *testing.T) {
 func TestServicePrincipalTokenRefreshUsesPOST(t *testing.T) {
 	spt := newServicePrincipalToken()
 
-	body := mocks.NewBody(newTokenJSON("test", "test"))
+	body := mocks.NewBody(newTokenJSON("12345", "test"))
 	resp := mocks.NewResponseWithBodyAndStatus(body, http.StatusOK, "OK")
 
 	c := mocks.NewSender()
@@ -199,7 +199,7 @@ func TestServicePrincipalTokenFromMSIRefreshUsesGET(t *testing.T) {
 		t.Fatalf("Failed to get MSI SPT: %v", err)
 	}
 
-	body := mocks.NewBody(newTokenJSON("test", "test"))
+	body := mocks.NewBody(newTokenJSON("12345", "test"))
 	resp := mocks.NewResponseWithBodyAndStatus(body, http.StatusOK, "OK")
 
 	c := mocks.NewSender()
@@ -264,7 +264,7 @@ func TestServicePrincipalTokenFromMSIRefreshCancel(t *testing.T) {
 func TestServicePrincipalTokenRefreshSetsMimeType(t *testing.T) {
 	spt := newServicePrincipalToken()
 
-	body := mocks.NewBody(newTokenJSON("test", "test"))
+	body := mocks.NewBody(newTokenJSON("12345", "test"))
 	resp := mocks.NewResponseWithBodyAndStatus(body, http.StatusOK, "OK")
 
 	c := mocks.NewSender()
@@ -291,7 +291,7 @@ func TestServicePrincipalTokenRefreshSetsMimeType(t *testing.T) {
 func TestServicePrincipalTokenRefreshSetsURL(t *testing.T) {
 	spt := newServicePrincipalToken()
 
-	body := mocks.NewBody(newTokenJSON("test", "test"))
+	body := mocks.NewBody(newTokenJSON("12345", "test"))
 	resp := mocks.NewResponseWithBodyAndStatus(body, http.StatusOK, "OK")
 
 	c := mocks.NewSender()
@@ -315,7 +315,7 @@ func TestServicePrincipalTokenRefreshSetsURL(t *testing.T) {
 }
 
 func testServicePrincipalTokenRefreshSetsBody(t *testing.T, spt *ServicePrincipalToken, f func(*testing.T, []byte)) {
-	body := mocks.NewBody(newTokenJSON("test", "test"))
+	body := mocks.NewBody(newTokenJSON("12345", "test"))
 	resp := mocks.NewResponseWithBodyAndStatus(body, http.StatusOK, "OK")
 
 	c := mocks.NewSender()
@@ -435,7 +435,7 @@ func TestServicePrincipalTokenSecretRefreshSetsBody(t *testing.T) {
 func TestServicePrincipalTokenRefreshClosesRequestBody(t *testing.T) {
 	spt := newServicePrincipalToken()
 
-	body := mocks.NewBody(newTokenJSON("test", "test"))
+	body := mocks.NewBody(newTokenJSON("12345", "test"))
 	resp := mocks.NewResponseWithBodyAndStatus(body, http.StatusOK, "OK")
 
 	c := mocks.NewSender()
@@ -460,7 +460,7 @@ func TestServicePrincipalTokenRefreshClosesRequestBody(t *testing.T) {
 func TestServicePrincipalTokenRefreshRejectsResponsesWithStatusNotOK(t *testing.T) {
 	spt := newServicePrincipalToken()
 
-	body := mocks.NewBody(newTokenJSON("test", "test"))
+	body := mocks.NewBody(newTokenJSON("12345", "test"))
 	resp := mocks.NewResponseWithBodyAndStatus(body, http.StatusUnauthorized, "Unauthorized")
 
 	c := mocks.NewSender()
@@ -559,7 +559,7 @@ func TestServicePrincipalTokenEnsureFreshRefreshes(t *testing.T) {
 	spt := newServicePrincipalToken()
 	expireToken(&spt.inner.Token)
 
-	body := mocks.NewBody(newTokenJSON("test", "test"))
+	body := mocks.NewBody(newTokenJSON("12345", "test"))
 	resp := mocks.NewResponseWithBodyAndStatus(body, http.StatusOK, "OK")
 
 	f := false

autorest/authorization_test.go

@@ -102,8 +102,8 @@ func TestServicePrincipalTokenWithAuthorizationRefresh(t *testing.T) {
 	jwt := `{
 		"access_token" : "accessToken",
 		"expires_in"   : "3600",
-		"expires_on"   : "test",
-		"not_before"   : "test",
+		"expires_on"   : "12345",
+		"not_before"   : "67890",
 		"resource"     : "test",
 		"token_type"   : "Bearer"
 	}`
@@ -317,35 +317,35 @@ func TestMultiTenantServicePrincipalTokenWithAuthorizationRefresh(t *testing.T)
 	primaryToken := `{
 		"access_token" : "primary token refreshed",
 		"expires_in"   : "3600",
-		"expires_on"   : "test",
-		"not_before"   : "test",
+		"expires_on"   : "12345",
+		"not_before"   : "67890",
 		"resource"     : "test",
 		"token_type"   : "Bearer"
 	}`
 
 	auxToken1 := `{
 		"access_token" : "aux token 1 refreshed",
 		"expires_in"   : "3600",
-		"expires_on"   : "test",
-		"not_before"   : "test",
+		"expires_on"   : "12345",
+		"not_before"   : "67890",
 		"resource"     : "test",
 		"token_type"   : "Bearer"
 	}`
 
 	auxToken2 := `{
 		"access_token" : "aux token 2 refreshed",
 		"expires_in"   : "3600",
-		"expires_on"   : "test",
-		"not_before"   : "test",
+		"expires_on"   : "12345",
+		"not_before"   : "67890",
 		"resource"     : "test",
 		"token_type"   : "Bearer"
 	}`
 
 	auxToken3 := `{
 		"access_token" : "aux token 3 refreshed",
 		"expires_in"   : "3600",
-		"expires_on"   : "test",
-		"not_before"   : "test",
+		"expires_on"   : "12345",
+		"not_before"   : "67890",
 		"resource"     : "test",
 		"token_type"   : "Bearer"
 	}`

autorest/preparer_test.go

@@ -158,10 +158,11 @@ func ExampleWithBaseURL() {
 	// Output: https://microsoft.com/a/b/c/
 }
 
-func ExampleWithBaseURL_second() {
+func TestWithBaseURL_second(t *testing.T) {
 	_, err := Prepare(&http.Request{}, WithBaseURL(":"))
-	fmt.Println(err)
-	// Output: parse :: missing protocol scheme
+	if err == nil {
+		t.Fatal("unexpected nil error")
+	}
 }
 
 // Create a request whose Body is a byte array
@@ -203,11 +204,12 @@ func ExampleWithCustomBaseURL() {
 	// Output: https://myaccount.blob.core.windows.net/
 }
 
-func ExampleWithCustomBaseURL_second() {
+func TestWithCustomBaseURL_second(t *testing.T) {
 	_, err := Prepare(&http.Request{},
 		WithCustomBaseURL(":", map[string]interface{}{}))
-	fmt.Println(err)
-	// Output: parse :: missing protocol scheme
+	if err == nil {
+		t.Fatal("unexpected nil error")
+	}
 }
 
 // Create a request with a custom HTTP header

autorest/version.go

@@ -19,7 +19,7 @@ import (
 	"runtime"
 )
 
-const number = "v14.0.0"
+const number = "v14.0.1"
 
 var (
 	userAgent = fmt.Sprintf("Go/%s (%s-%s) go-autorest/%s",

azure-pipelines.yml

@@ -1,82 +1,103 @@
-pool:
-  vmImage: 'Ubuntu 16.04'
-
 variables:
-  GOROOT: '/usr/local/go1.12'
   GOPATH: '$(system.defaultWorkingDirectory)/work'
   sdkPath: '$(GOPATH)/src/github.com/$(build.repository.name)'
 
-steps:
-- script: |
-    set -e
-    mkdir -p '$(GOPATH)/bin'
-    mkdir -p '$(sdkPath)'
-    shopt -s extglob
-    mv !(work) '$(sdkPath)'
-    echo '##vso[task.prependpath]$(GOROOT)/bin'
-    echo '##vso[task.prependpath]$(GOPATH)/bin'
-  displayName: 'Create Go Workspace'
-- script: |
-    set -e
-    curl -sSL https://raw.githubusercontent.com/golang/dep/master/install.sh | sh
-    dep ensure -v
-    go install ./vendor/golang.org/x/lint/golint
-    go get github.com/jstemmer/go-junit-report
-    go get github.com/axw/gocov/gocov
-    go get github.com/AlekSi/gocov-xml
-    go get -u github.com/matm/gocov-html
-  workingDirectory: '$(sdkPath)'
-  displayName: 'Install Dependencies'
-- script: |
-    go vet ./autorest/...
-    go vet ./logger/...
-    go vet ./tracing/...
-  workingDirectory: '$(sdkPath)'
-  displayName: 'Vet'
-- script: |
-    go build -v ./autorest/...
-    go build -v ./logger/...
-    go build -v ./tracing/...
-  workingDirectory: '$(sdkPath)'
-  displayName: 'Build'
-- script: |
-    set -e
-    go test -race -v -coverprofile=coverage.txt -covermode atomic ./autorest/... ./logger/... ./tracing/... 2>&1 | go-junit-report > report.xml
-    gocov convert coverage.txt > coverage.json
-    gocov-xml < coverage.json > coverage.xml
-    gocov-html < coverage.json > coverage.html
-  workingDirectory: '$(sdkPath)'
-  displayName: 'Run Tests'
-- script: grep -L -r --include *.go --exclude-dir vendor -P "Copyright (\d{4}|\(c\)) Microsoft" ./ | tee >&2
-  workingDirectory: '$(sdkPath)'
-  displayName: 'Copyright Header Check'
-  failOnStderr: true
-  condition: succeededOrFailed()
-- script: |
-    gofmt -s -l -w ./autorest/. >&2
-    gofmt -s -l -w ./logger/. >&2
-    gofmt -s -l -w ./tracing/. >&2
-  workingDirectory: '$(sdkPath)'
-  displayName: 'Format Check'
-  failOnStderr: true
-  condition: succeededOrFailed()
-- script: |
-    golint ./autorest/... >&2
-    golint ./logger/... >&2
-    golint ./tracing/... >&2
-  workingDirectory: '$(sdkPath)'
-  displayName: 'Linter Check'
-  failOnStderr: true
-  condition: succeededOrFailed()
+jobs:
+  - job: 'goautorest'
+    displayName: 'Run go-autorest CI Checks'
+
+    strategy:
+      matrix:
+        Linux_Go113:
+          vm.image: 'ubuntu-18.04'
+          go.version: '1.13'
+          GOROOT: '/usr/local/go$(go.version)'
+        Linux_Go114:
+          vm.image: 'ubuntu-18.04'
+          go.version: '1.14'
+          GOROOT: '/usr/local/go$(go.version)'
+
+    pool:
+      vmImage: '$(vm.image)'
+
+    steps:
+      - script: |
+          set -e
+          mkdir -p '$(GOPATH)/bin'
+          mkdir -p '$(sdkPath)'
+          shopt -s extglob
+          mv !(work) '$(sdkPath)'
+          echo '##vso[task.prependpath]$(GOROOT)/bin'
+          echo '##vso[task.prependpath]$(GOPATH)/bin'
+        displayName: 'Create Go Workspace'
+
+      - script: |
+          set -e
+          curl -sSL https://raw.githubusercontent.com/golang/dep/master/install.sh | sh
+          dep ensure -v
+          go install ./vendor/golang.org/x/lint/golint
+          go get github.com/jstemmer/go-junit-report
+          go get github.com/axw/gocov/gocov
+          go get github.com/AlekSi/gocov-xml
+          go get -u github.com/matm/gocov-html
+        workingDirectory: '$(sdkPath)'
+        displayName: 'Install Dependencies'
+
+      - script: |
+          go vet ./autorest/...
+          go vet ./logger/...
+          go vet ./tracing/...
+        workingDirectory: '$(sdkPath)'
+        displayName: 'Vet'
+
+      - script: |
+          go build -v ./autorest/...
+          go build -v ./logger/...
+          go build -v ./tracing/...
+        workingDirectory: '$(sdkPath)'
+        displayName: 'Build'
+
+      - script: |
+          set -e
+          go test -race -v -coverprofile=coverage.txt -covermode atomic ./autorest/... ./logger/... ./tracing/... 2>&1 | go-junit-report > report.xml
+          gocov convert coverage.txt > coverage.json
+          gocov-xml < coverage.json > coverage.xml
+          gocov-html < coverage.json > coverage.html
+        workingDirectory: '$(sdkPath)'
+        displayName: 'Run Tests'
+
+      - script: grep -L -r --include *.go --exclude-dir vendor -P "Copyright (\d{4}|\(c\)) Microsoft" ./ | tee >&2
+        workingDirectory: '$(sdkPath)'
+        displayName: 'Copyright Header Check'
+        failOnStderr: true
+        condition: succeededOrFailed()
+
+      - script: |
+          gofmt -s -l -w ./autorest/. >&2
+          gofmt -s -l -w ./logger/. >&2
+          gofmt -s -l -w ./tracing/. >&2
+        workingDirectory: '$(sdkPath)'
+        displayName: 'Format Check'
+        failOnStderr: true
+        condition: succeededOrFailed()
+
+      - script: |
+          golint ./autorest/... >&2
+          golint ./logger/... >&2
+          golint ./tracing/... >&2
+        workingDirectory: '$(sdkPath)'
+        displayName: 'Linter Check'
+        failOnStderr: true
+        condition: succeededOrFailed()
 
-- task: PublishTestResults@2
-  inputs:
-    testRunner: JUnit
-    testResultsFiles: $(sdkPath)/report.xml
-    failTaskOnFailedTests: true
+      - task: PublishTestResults@2
+        inputs:
+          testRunner: JUnit
+          testResultsFiles: $(sdkPath)/report.xml
+          failTaskOnFailedTests: true
 
-- task: PublishCodeCoverageResults@1
-  inputs:
-    codeCoverageTool: Cobertura 
-    summaryFileLocation: $(sdkPath)/coverage.xml
-    additionalCodeCoverageFiles: $(sdkPath)/coverage.html
+      - task: PublishCodeCoverageResults@1
+        inputs:
+          codeCoverageTool: Cobertura 
+          summaryFileLocation: $(sdkPath)/coverage.xml
+          additionalCodeCoverageFiles: $(sdkPath)/coverage.html