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.

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

[Test][core] inline build:test commands into browser test scripts #29411

Merged

Conversation

jeremymeng
Copy link
Contributor

@jeremymeng jeremymeng commented Apr 23, 2024

As far as I know for NodeJS the majority of our packages don't need build:test as we either runs test on .ts files, or build script already generates the .js files under dist-esm. Currently we only need to build the test for browser testing.

This PR moves commands in build:test into the browser test scripts for core packages, thus saving us from rebuilding when testing for NodeJS.

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@@ -57,7 +57,7 @@
],
"scripts": {
"build:samples": "echo Obsolete",
"build:test": "npm run clean && tshy && dev-tool run build-test",
"build:test": "echo skipped. actual commands inlined in browser test scripts",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

another approach is to have build:test:node and build:test:browser but it seems that we used to have that but moved away from it.

As far as I know for the majority of our packages we don't need to run
`build:test` as we either runs test on .ts files, or `build` script already
generates the .js files under `dist-esm`. Currently we only need to build the
test for browser testing.

This PR moves commands in `build:test` into the browser test scripts, thus
saving us from rebuilding when testing for NodeJS.
@jeremymeng jeremymeng force-pushed the test/core-only-build-test-for-browser branch from efb1f38 to fcec45d Compare April 23, 2024 21:05
Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

I love the idea of getting away from this step.

As a follow-up, can we roll this change out to all packages and remove the command from our CI?

node eng/tools/rush-runner.js build:test "${{parameters.ServiceDirectory}}" -packages "$(ArtifactPackageNames)" --verbose -p max

and

node common/scripts/install-run-rush.js build:test -t "${{parameters.PackageName}}" --verbose -p max

@jeremymeng
Copy link
Contributor Author

As a follow-up, can we roll this change out to all packages and remove the command from our CI?

yeah that's what I eventually want too. I had a failed attempt earlier because some packages still depends on build:test. Once we get them to all "echo skipped" then the steps can be removed.

@jeremymeng jeremymeng merged commit 4e3aba1 into Azure:main May 2, 2024
34 checks passed
@jeremymeng jeremymeng deleted the test/core-only-build-test-for-browser branch May 2, 2024 21:48
jeremymeng added a commit to jeremymeng/azure-sdk-for-js that referenced this pull request May 8, 2024
for non-core packages in js - core pipelines.

This PR is a follow-up of PR
Azure#29411 for non-core packages.
jeremymeng added a commit that referenced this pull request May 10, 2024
for non-core packages in js - core pipelines.

This PR is a follow-up of PR
#29411 for non-core
packages.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants