Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Container Registry] Migrate to new recorder #20301

Merged
merged 13 commits into from
Feb 11, 2022
Merged

[Container Registry] Migrate to new recorder #20301

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
2510424
[Container Registry] Migrate to new recorder
timovv Jan 31, 2022
a4f6a63
Update sample.env to reflect current situation
timovv Feb 10, 2022
95c540d
Add browser tests and sanitize better
timovv Feb 10, 2022
4f11c49
Remove redundant dotenv helpers
timovv Feb 10, 2022
4cff5f0
TestProxy=true
timovv Feb 10, 2022
cac8977
Add additional options to createTestCredential
timovv Feb 10, 2022
ea35749
Revert environment variable changes
timovv Feb 10, 2022
d09107c
fix oops
timovv Feb 10, 2022
863f25f
Merge branch 'main' into container-registry/19859-recorder-migration
timovv Feb 10, 2022
0a09d76
Remove now-redundant TestProxy: true CI option
timovv Feb 10, 2022
644f623
Sort package.json
timovv Feb 10, 2022
e61212b
Merge branch 'main' into container-registry/19859-recorder-migration
timovv Feb 11, 2022
43f515a
Fixes after merge
timovv Feb 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions sdk/containerregistry/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ pr:
extends:
template: ../../eng/pipelines/templates/stages/archetype-sdk-client.yml
parameters:
TestProxy: true
timovv marked this conversation as resolved.
Show resolved Hide resolved
ServiceDirectory: containerregistry
Artifacts:
- name: azure-container-registry
Expand Down
30 changes: 8 additions & 22 deletions sdk/containerregistry/container-registry/karma.conf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,9 @@

// https://github.com/karma-runner/karma-chrome-launcher
process.env.CHROME_BIN = require("puppeteer").executablePath();
process.env.RECORDINGS_RELATIVE_PATH =
require("@azure-tools/test-recorder").relativeRecordingsPath();
require("dotenv").config();
const {
jsonRecordingFilterFunction,
isPlaybackMode,
isSoftRecordMode,
isRecordMode,
} = require("@azure-tools/test-recorder");

module.exports = function (config) {
config.set({
Expand All @@ -31,15 +27,13 @@ module.exports = function (config) {
"karma-coverage",
"karma-sourcemap-loader",
"karma-junit-reporter",
"karma-json-to-file-reporter",
"karma-json-preprocessor",
],

// list of files / patterns to load in the browser
files: [
"dist-test/index.browser.js",
{ pattern: "dist-test/index.browser.js.map", type: "html", included: false, served: true },
].concat(isPlaybackMode() || isSoftRecordMode() ? ["recordings/browsers/**/*.json"] : []),
],

// list of files / patterns to exclude
exclude: [],
Expand All @@ -48,7 +42,6 @@ module.exports = function (config) {
// available preprocessors: https://npmjs.org/browse/keyword/karma-preprocessor
preprocessors: {
"**/*.js": ["sourcemap", "env"],
"recordings/browsers/**/*.json": ["json"],
// IMPORTANT: COMMENT following line if you want to debug in your browsers!!
// Preprocess source file to calculate code coverage, however this will make source file unreadable
//"dist-test/index.browser.js": ["coverage"]
Expand All @@ -58,15 +51,16 @@ module.exports = function (config) {
"TEST_MODE",
"CONTAINER_REGISTRY_ENDPOINT",
"CONTAINER_REGISTRY_ANONYMOUS_ENDPOINT",
"CONTAINERREGISTRY_CLIENT_ID",
"CONTAINERREGISTRY_CLIENT_SECRET",
"CONTAINERREGISTRY_TENANT_ID",
"AZURE_CLIENT_ID",
"AZURE_CLIENT_SECRET",
"AZURE_TENANT_ID",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I created the weekly live test pipeline, AZURE_*_* env vars didn't work for other clouds because they were set to the Public cloud's client/tenant. It is likely to be fixed now. Please trigger the js - container-registry - tests-weekly pipeline to make sure it's working. https://dev.azure.com/azure-sdk/internal/_build?definitionId=2886

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll trigger that soon. I changed these back to their usual names so that the createTestCredential helper could be used -- it uses the standard names when creating the credential. If the issue still exists, then I can revert to using the CONTAINERREGISTRY_* names and switch between NoOpCredential and the DefaultAzureCredential without the helper.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know this.

Maybe update the test-credential package to take more options?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appears to be failing, I'll add an option to test-credential.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to fix the pipeline instead? I thought it is expected that Azure_* is set to be the ones from the cloud being tests against. @benbp

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not blocking this PR. But I'd love to not special case ACR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change to add options to the test credential is nice to have anyway IMO regardless of whether we fix the underlying issue here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have not historically set AZURE_* variables and instead set namespaced variables like CONTAINERREGISTRY_* in the test infastructure to avoid potential collisions when running test batches. I have an issue to fix this though, as in practice it hasn't been an issue. A current workaround is to set a redirect in the pipeline definition, e.g. here.

        AZURE_CLIENT_ID: $(CONTAINERREGISTRY_CLIENT_ID)
        AZURE_TENANT_ID: $(CONTAINERREGISTRY_TENANT_ID)
        AZURE_CLIENT_SECRET: $(CONTAINERREGISTRY_CLIENT_SECRET)
        AZURE_SUBSCRIPTION_ID: $(CONTAINERREGISTRY_SUBSCRIPTION_ID)

"RECORDINGS_RELATIVE_PATH",
],

// test results reporter to use
// possible values: 'dots', 'progress'
// available reporters: https://npmjs.org/browse/keyword/karma-reporter
reporters: ["mocha", "coverage", "junit", "json-to-file"],
reporters: ["mocha", "coverage", "junit"],

coverageReporter: {
// specify a common output directory
Expand All @@ -84,11 +78,6 @@ module.exports = function (config) {
properties: {}, // key value pair of properties to add to the <properties> section of the report
},

jsonToFileReporter: {
filter: jsonRecordingFilterFunction,
outputPath: ".",
},

// web server port
port: 9876,

Expand Down Expand Up @@ -123,9 +112,6 @@ module.exports = function (config) {
browserNoActivityTimeout: 600000,
browserDisconnectTimeout: 10000,
browserDisconnectTolerance: 3,
browserConsoleLogOptions: {
terminal: !isRecordMode(),
},

client: {
mocha: {
Expand Down
15 changes: 7 additions & 8 deletions sdk/containerregistry/container-registry/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,17 +36,17 @@
"extract-api": "tsc -p . && api-extractor run --local",
"format": "prettier --write --config ../../../.prettierrc.json --ignore-path ../../../.prettierignore \"src/**/*.ts\" \"test/**/*.ts\" \"samples-dev/**/*.ts\" \"*.{js,json}\"",
"generate:client": "autorest --typescript ./swagger/README.md",
"integration-test:browser": "karma start --single-run",
"integration-test:node": "nyc mocha -r esm --require source-map-support/register --reporter ../../../common/tools/mocha-multi-reporter.js --timeout 5000000 --full-trace \"dist-esm/test/{,!(browser)/**/}/*.spec.js\"",
"integration-test:browser": "dev-tool run test:browser",
"integration-test:node": "dev-tool run test:node-js-input -- --timeout 5000000 --full-trace \"dist-esm/test/**/*.spec.js\"",
"integration-test": "npm run integration-test:node && npm run integration-test:browser",
"lint:fix": "eslint package.json api-extractor.json src test --ext .ts --fix --fix-type [problem,suggestion]",
"lint": "eslint package.json api-extractor.json src test --ext .ts",
"pack": "npm pack 2>&1",
"test:browser": "npm run build:test && npm run unit-test:browser && npm run integration-test:browser",
"test:node": "npm run build:test && npm run unit-test:node && npm run integration-test:node",
"test": "npm run build:test && npm run unit-test && npm run integration-test",
"unit-test:browser": "karma start --single-run",
"unit-test:node": "mocha -r esm --require ts-node/register --reporter ../../../common/tools/mocha-multi-reporter.js --timeout 1200000 --full-trace \"test/{,!(browser)/**/}/*.spec.ts\"",
"unit-test:browser": "dev-tool run test:browser",
"unit-test:node": "dev-tool run test:node-ts-input -- --timeout 1200000 --full-trace \"test/**/*.spec.ts\"",
"unit-test": "npm run unit-test:node && npm run unit-test:browser"
},
"files": [
Expand Down Expand Up @@ -87,7 +87,7 @@
"@azure/eslint-plugin-azure-sdk": "^3.0.0",
"@azure/identity": "^2.0.1",
"@azure/test-utils": "^1.0.0",
"@azure-tools/test-recorder": "^1.0.0",
"@azure-tools/test-recorder": "^2.0.0",
"@microsoft/api-extractor": "^7.18.11",
"@types/chai": "^4.1.6",
"@types/chai-as-promised": "^7.1.0",
Expand All @@ -106,8 +106,6 @@
"karma-env-preprocessor": "^0.1.1",
"karma-firefox-launcher": "^1.1.0",
"karma-ie-launcher": "^1.0.0",
"karma-json-preprocessor": "^0.3.3",
"karma-json-to-file-reporter": "^1.0.1",
"karma-junit-reporter": "^2.0.1",
"karma-mocha": "^2.0.1",
"karma-mocha-reporter": "^2.2.5",
Expand All @@ -118,7 +116,8 @@
"prettier": "^2.5.1",
"rimraf": "^3.0.0",
"typescript": "~4.2.0",
"util": "^0.12.1"
"util": "^0.12.1",
"@azure-tools/test-credential": "^1.0.0"
timovv marked this conversation as resolved.
Show resolved Hide resolved
},
"//sampleConfiguration": {
"skipFolder": true,
Expand Down
220 changes: 147 additions & 73 deletions ...sers/anonymous_access_tests/recording_should_list_repositories_with_anonymous_access.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading