-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
[recorder] Rename RECORDING_ASSETS_PATH environment variable #24751
Conversation
…S_PATH for consistency
@@ -50,7 +50,7 @@ module.exports = function (config) { | |||
// inject following environment values into browser testing with window.__env__ | |||
// environment values MUST be exported or set with same console running "karma start" | |||
// https://www.npmjs.com/package/karma-env-preprocessor | |||
envPreprocessor: ["RECORDINGS_RELATIVE_PATH", "PROXY_MANUAL_START", "RECORDING_ASSETS_PATH"], | |||
envPreprocessor: ["RECORDINGS_RELATIVE_PATH", "PROXY_MANUAL_START", "RECORDINGS_ASSETS_PATH"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why can't we rely on the RECORDINGS_RELATIVE_PATH for RECORDINGS_ASSETS_PATH?
Now that the assets path is fixed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assets.json might not be present, in which case the RECORDING_ASSETS_PATH is left undefined. There's no way of detecting whether it's there in the browser. I experimented with it in the previous PR: #23405 (review). But it didn't work because of that reason.
If we can come up with a better solution I would be thrilled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, we could catch the error the test proxy throws when the provided assets.json doesn't exist and try again without providing it. That could work! I'll give that a go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assets.json might not be present, in which case the RECORDING_ASSETS_PATH is left undefined. There's no way of detecting whether it's there in the browser. I experimented with it in the previous PR: #23405 (review). But it didn't work because of that reason.
oh yeah, that makes sense, I remember that now from Scott's PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, we could catch the error the test proxy throws when the provided assets.json doesn't exist and try again without providing it. That could work! I'll give that a go.
I'm thrilled, but then all the packages have to be migrated at once, right?
If they don't have the assets.json, they'll fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The start request would fail initially. We'd look at the failure and if it's because the assets.json doesn't exist we'd make the request again without specifying an assets.json. So it wouldn't be a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful.
Closing in favor of #24776. |
Packages impacted by this PR
@azure-tools/test-recorder
Describe the problem addressed by this PR
Changing the variable name (adding a s at the end of "recording") for consistency with pre-existing
RECORDINGS_RELATIVE_PATH
variable.