From 2827e1ec6410ee04f580f90ab16624da167fc0c9 Mon Sep 17 00:00:00 2001 From: Ben Broderick Phillips Date: Thu, 20 Oct 2022 17:21:36 -0400 Subject: [PATCH] Add support for making recordings external to the repository (#19322) * Improve error and pipeline handling in run_tests.ps1 * Add test proxy default assets directory to gitignore * Support test proxy external asset mode for test recordings * Handle recording asset sync better for different go test working directories * Handle relative and absolute paths depending on test proxy working directory * Use git executable to detect git root in test recording handler * Improve os-aware path handling in test recording framework asset sync * Cast git response to string * Simplify git and parent path checks * Bump sdk/internal package version --- .gitignore | 5 +- eng/scripts/run_tests.ps1 | 11 ++- sdk/internal/CHANGELOG.md | 8 +- sdk/internal/recording/recording.go | 119 ++++++++++++++++++++--- sdk/internal/recording/recording_test.go | 63 ++++++++++++ sdk/internal/version.go | 2 +- 6 files changed, 182 insertions(+), 26 deletions(-) diff --git a/.gitignore b/.gitignore index 40e51d5c4a72..61f01a7c59cf 100644 --- a/.gitignore +++ b/.gitignore @@ -49,4 +49,7 @@ vendor/ !.vscode/cspell.json # api view file -*.gosource \ No newline at end of file +*.gosource + +# Default Test Proxy Assets restore directory +.assets diff --git a/eng/scripts/run_tests.ps1 b/eng/scripts/run_tests.ps1 index 2b7feaa968d9..0181e6f1b10f 100644 --- a/eng/scripts/run_tests.ps1 +++ b/eng/scripts/run_tests.ps1 @@ -1,10 +1,13 @@ #Requires -Version 7.0 Param( + [Parameter(Mandatory = $true)] [string] $serviceDirectory, - [string] $testTimeout + [string] $testTimeout = "10s" ) +$ErrorActionPreference = 'Stop' + Push-Location sdk/$serviceDirectory Write-Host "##[command] Executing 'go test -timeout $testTimeout -v -coverprofile coverage.txt ./...' in sdk/$serviceDirectory" @@ -12,7 +15,7 @@ go test -timeout $testTimeout -v -coverprofile coverage.txt ./... | Tee-Object - # go test will return a non-zero exit code on test failures so don't skip generating the report in this case $GOTESTEXITCODE = $LASTEXITCODE -Get-Content outfile.txt | go-junit-report > report.xml +Get-Content -Raw outfile.txt | go-junit-report > report.xml # if no tests were actually run (e.g. examples) delete the coverage file so it's omitted from the coverage report if (Select-String -path ./report.xml -pattern '' -simplematch -quiet) { @@ -32,8 +35,8 @@ if (Select-String -path ./report.xml -pattern '' -simpl Get-Content ./coverage.json | gocov-xml > ./coverage.xml Get-Content ./coverage.json | gocov-html > ./coverage.html - Move-Item ./coverage.xml $repoRoot - Move-Item ./coverage.html $repoRoot + Move-Item -Force ./coverage.xml $repoRoot + Move-Item -Force ./coverage.html $repoRoot # use internal tool to fail if coverage is too low Pop-Location diff --git a/sdk/internal/CHANGELOG.md b/sdk/internal/CHANGELOG.md index abad8b877bf6..2689134d2db1 100644 --- a/sdk/internal/CHANGELOG.md +++ b/sdk/internal/CHANGELOG.md @@ -1,14 +1,10 @@ # Release History -## 1.0.2 (Unreleased) +## 1.1.0 (2022-10-20) ### Features Added -### Breaking Changes - -### Bugs Fixed - -### Other Changes +* Support test recording assets external to repository ## 1.0.1 (2022-08-22) diff --git a/sdk/internal/recording/recording.go b/sdk/internal/recording/recording.go index e7ee2bde3fa3..0c0eebcf4911 100644 --- a/sdk/internal/recording/recording.go +++ b/sdk/internal/recording/recording.go @@ -14,12 +14,12 @@ import ( "errors" "fmt" "io" + "io/fs" "log" "math/rand" "net/http" "os" "os/exec" - "path" "path/filepath" "runtime" "strconv" @@ -54,6 +54,7 @@ const ( randomSeedVariableName = "randomSeed" nowVariableName = "now" ModeEnvironmentVariableName = "AZURE_TEST_MODE" + recordingAssetConfigName = "assets.json" ) // Inspired by https://stackoverflow.com/questions/22892120/how-to-generate-a-random-string-of-a-fixed-length-in-go @@ -574,7 +575,95 @@ func (r RecordingOptions) baseURL() string { } func getTestId(pathToRecordings string, t *testing.T) string { - return path.Join(pathToRecordings, "recordings", t.Name()+".json") + return filepath.Join(pathToRecordings, "recordings", t.Name()+".json") +} + +func getGitRoot(fromPath string) (string, error) { + absPath, err := filepath.Abs(fromPath) + if err != nil { + return "", err + } + cmd := exec.Command("git", "rev-parse", "--show-toplevel") + cmd.Dir = absPath + + root, err := cmd.CombinedOutput() + if err != nil { + return "", fmt.Errorf("Unable to find git root for path '%s'", absPath) + } + + // Wrap with Abs() to get os-specific path separators to support sub-path matching + return filepath.Abs(strings.TrimSpace(string(root))) +} + +// Traverse up from a recording path until an asset config file is found. +// Stop searching when the root of the git repository is reached. +func findAssetsConfigFile(fromPath string, untilPath string) (string, error) { + absPath, err := filepath.Abs(fromPath) + if err != nil { + return "", err + } + assetConfigPath := filepath.Join(absPath, recordingAssetConfigName) + + if _, err := os.Stat(assetConfigPath); err == nil { + return assetConfigPath, nil + } else if !errors.Is(err, fs.ErrNotExist) { + return "", err + } + + if absPath == untilPath { + return "", nil + } + + parentDir := filepath.Dir(absPath) + // This shouldn't be hit due to checks in getGitRoot, but it can't hurt to be defensive + if parentDir == absPath || parentDir == "." { + return "", nil + } + + return findAssetsConfigFile(parentDir, untilPath) +} + +// Returns absolute and relative paths to an asset configuration file, or an error. +func getAssetsConfigLocation(pathToRecordings string) (string, string, error) { + cwd, err := os.Getwd() + if err != nil { + return "", "", err + } + gitRoot, err := getGitRoot(cwd) + if err != nil { + return "", "", err + } + abs, err := findAssetsConfigFile(filepath.Join(gitRoot, pathToRecordings), gitRoot) + if err != nil { + return "", "", err + } + + // Pass a path relative to the git root to test proxy so that paths + // can be resolved when the repo root is mounted as a volume in a container + rel := strings.Replace(abs, gitRoot, "", 1) + rel = strings.TrimLeft(rel, string(os.PathSeparator)) + return abs, rel, nil +} + +func requestStart(url string, testId string, assetConfigLocation string) (*http.Response, error) { + req, err := http.NewRequest("POST", url, nil) + if err != nil { + return nil, err + } + + req.Header.Set("Content-Type", "application/json") + reqBody := map[string]string{"x-recording-file": testId} + if assetConfigLocation != "" { + reqBody["x-recording-assets-file"] = assetConfigLocation + } + marshalled, err := json.Marshal(reqBody) + if err != nil { + return nil, err + } + req.Body = io.NopCloser(bytes.NewReader(marshalled)) + req.ContentLength = int64(len(marshalled)) + + return client.Do(req) } // Start tells the test proxy to begin accepting requests for a given test @@ -595,25 +684,27 @@ func Start(t *testing.T, pathToRecordings string, options *RecordingOptions) err testId := getTestId(pathToRecordings, t) - url := fmt.Sprintf("%s/%s/start", options.baseURL(), recordMode) - - req, err := http.NewRequest("POST", url, nil) + absAssetLocation, relAssetLocation, err := getAssetsConfigLocation(pathToRecordings) if err != nil { return err } - req.Header.Set("Content-Type", "application/json") - marshalled, err := json.Marshal(map[string]string{"x-recording-file": testId}) - if err != nil { - return err - } - req.Body = io.NopCloser(bytes.NewReader(marshalled)) - req.ContentLength = int64(len(marshalled)) + url := fmt.Sprintf("%s/%s/start", options.baseURL(), recordMode) - resp, err := client.Do(req) - if err != nil { + var resp *http.Response + if absAssetLocation == "" { + resp, err = requestStart(url, testId, "") + if err != nil { + return err + } + } else if resp, err = requestStart(url, testId, absAssetLocation); err != nil { return err + } else if resp.StatusCode >= 400 { + if resp, err = requestStart(url, testId, relAssetLocation); err != nil { + return err + } } + recId := resp.Header.Get(IDHeader) if recId == "" { b, err := io.ReadAll(resp.Body) diff --git a/sdk/internal/recording/recording_test.go b/sdk/internal/recording/recording_test.go index 9e1c1eceb943..233df18b1286 100644 --- a/sdk/internal/recording/recording_test.go +++ b/sdk/internal/recording/recording_test.go @@ -550,6 +550,69 @@ func TestHostAndScheme(t *testing.T) { require.Equal(t, r.host(), "localhost:5000") } +func TestGitRootDetection(t *testing.T) { + cwd, err := os.Getwd() + require.NoError(t, err) + gitRoot, err := getGitRoot(cwd) + require.NoError(t, err) + + parentDir := filepath.Dir(gitRoot) + _, err = getGitRoot(parentDir) + require.Error(t, err) +} + +func TestRecordingAssetConfigNotExist(t *testing.T) { + absPath, relPath, err := getAssetsConfigLocation(".") + require.NoError(t, err) + require.Equal(t, "", absPath) + require.Equal(t, "", relPath) +} + +func TestRecordingAssetConfigOutOfBounds(t *testing.T) { + cwd, err := os.Getwd() + require.NoError(t, err) + gitRoot, err := getGitRoot(cwd) + require.NoError(t, err) + parentDir := filepath.Dir(gitRoot) + + absPath, err := findAssetsConfigFile(parentDir, gitRoot) + require.NoError(t, err) + require.Equal(t, "", absPath) +} + +func TestRecordingAssetConfig(t *testing.T) { + cases := []struct{ expectedDirectory, searchDirectory, testFileLocation string }{ + {"sdk/internal/recording", "sdk/internal/recording", recordingAssetConfigName}, + {"sdk/internal/recording", "sdk/internal/recording/", recordingAssetConfigName}, + {"sdk/internal", "sdk/internal/recording", "../" + recordingAssetConfigName}, + {"sdk/internal", "sdk/internal/recording/", "../" + recordingAssetConfigName}, + } + + cwd, err := os.Getwd() + require.NoError(t, err) + gitRoot, err := getGitRoot(cwd) + require.NoError(t, err) + + for _, c := range cases { + _ = os.Remove(c.testFileLocation) + o, err := os.Create(c.testFileLocation) + require.NoError(t, err) + o.Close() + + absPath, relPath, err := getAssetsConfigLocation(c.searchDirectory) + // Clean up first in case of an assertion panic + require.NoError(t, os.Remove(c.testFileLocation)) + require.NoError(t, err) + + expected := c.expectedDirectory + string(os.PathSeparator) + recordingAssetConfigName + expected = strings.ReplaceAll(expected, "/", string(os.PathSeparator)) + require.Equal(t, expected, relPath) + + absPathExpected := filepath.Join(gitRoot, expected) + require.Equal(t, absPathExpected, absPath) + } +} + func TestFindProxyCertLocation(t *testing.T) { savedValue, ok := os.LookupEnv("PROXY_CERT") if ok { diff --git a/sdk/internal/version.go b/sdk/internal/version.go index f7458f8f5809..5cefa9894960 100644 --- a/sdk/internal/version.go +++ b/sdk/internal/version.go @@ -11,5 +11,5 @@ const ( Module = "internal" // Version is the semantic version (see http://semver.org) of this module. - Version = "v1.0.2" + Version = "v1.1.0" )