[attestation] Migrate to ESM and tshy #31433
Conversation
github-actions
bot
added
Attestation
dev-tool
|
/azp run js - attestation - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
API change check API changes are not detected in this pull request. |
| @@ -266,7 +266,7 @@ function setScriptsSection(scripts: PackageJson["scripts"]): void { | |||
| scripts["build"] = "npm run clean && dev-tool run build-package && dev-tool run extract-api"; | |||
|
|
|||
| scripts["unit-test:browser"] = | |||
| "npm run clean && dev-tool run build-package && dev-tool run build-test && dev-tool run test:vitest --no-test-proxy --browser"; | |||
| "npm run clean && dev-tool run build-package && dev-tool run build-test && dev-tool run test:vitest --browser"; | |||
There was a problem hiding this comment.
Would we assume the test-proxy is required for unit tests as well as for integration tests?
There was a problem hiding this comment.
I think the no-test-proxy flag should be reserved for packages that don’t use recordings at all, but I could be overlooking something.
Currently, the TEST_MODE environment variable seems to guide whether we use the proxy or not:
- playback (default): true
- recording: true
- live: false
However, the challenge I see is the distinction between unit-test and integration-test. This split seems to come from when we used to run unit-test from source code and integration-test from the distribution (JavaScript). But now that we’ve consolidated everything under Vitest, it raises the question: should integration-test only be used for live tests now? Or should we just rely on TEST_MODE entirely?
Another option could be to remove the integration-test script altogether, as running TEST_MODE=live rushx unit-test would achieve the same result as the current integration-test. This might be something worth discussing with the team, as the purpose behind keeping both scripts isn't entirely clear to me anymore.
idk, what do folks think?
| @@ -28,8 +28,8 @@ import { DefaultAzureCredential } from "@azure/identity"; | |||
|
|
|||
| // Load environment from a .env file if it exists. | |||
| import * as dotenv from "dotenv"; | |||
There was a problem hiding this comment.
Nit, but I wonder if we could rewrite import "dotenv/config";
maorleger
requested review from
a team,
jeremymeng,
timovv,
ckairen,
LarryOsterman and
deyaaeldeen
as code owners
|
/azp run js - attestation - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Packages impacted by this PR
@azure/attestation
Issues associated with this PR
#31338
Describe the problem that is addressed by this PR
Migrates
@azure/attestationto ESM and tshy. This took under an hour of active work!