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

build: migrate @angular-devkit/build-angular tests to rules_js #29502

Merged
merged 1 commit into from
Jan 29, 2025

Conversation

devversion
Copy link
Member

@devversion devversion commented Jan 28, 2025

Migrates the @angular-devkit/build-angular tests to rules_js. This was a rather larger undertaking as the tests were very reliant on e.g. the directory structure or specific node module layout; so some changes were needed.

  • the Sass files include a much larger file header now. That is because the npm Sass files have much larger paths, given being inside a symlinked pnpm store directory. E.g.
/*!**********************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************!*\
  !*** css ../../../../../node_modules/.aspect_rules_js/[email protected]_webpack_5.97.1/node_modules/css-loader/dist/cjs.js??ruleSet[1].rules[4].rules[0].oneOf[0].use[1]!../../../../../node_modules/.aspect_rules_js/[email protected]_1462687623/node_modules/postcss-loader/dist/cjs.js??ruleSet[1].rules[4].rules[0].oneOf[0].use[2]!./src/test-style-a.css?ngGlobalStyle ***!
  \**********************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************/
.test-a {color: red}
  • Similarly to above, hashed chunk files can change given different paths of e.g. Webpack, or external sources.

  • Tests for verifying the lazy module chunks may enable preserveSymlinks just to make the chunk names shorter and easier to verify, avoiding truncatd super long paths to the e.g. pnpm stores again.

  • the ngsw-worker.js file cannot be copied using copyFile as that results in permissions being copied as well. In Bazel, now that the npm files are properly captured, are readonly, so subsequent builds (e.g. the watch tests) will fail to copy/override the file again! Reading and writing the file consistently seems appropriate.

  • Tests relying on puppeteer and webdriver-manager worked in the past, by accident, because postinstall scripts (from e.g. puppeteer) were able to modify content of other packages (e.g. the puppeteer-core cache of browsers then). This does not work with rules_js anymore, so we need to keep the cache local to the puppeteer postinstall script. This requires a little trickery right now to ensure resolution of the browsers at runtime works..

  • server tests did miss the node types to be explicitly listed (as they would be in a fresh project), and this caused failures. Likely because we no longer patch resolution.

  • avoid npm-module style imports from tests within the same package. This is not allowed with rules_js and also is inconsistent.

@devversion devversion added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release labels Jan 28, 2025
@angular-robot angular-robot bot added the area: build & ci Related the build and CI infrastructure of the project label Jan 28, 2025
@alan-agius4 alan-agius4 added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Jan 28, 2025
Copy link
Collaborator

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

LGTM, one NIT

@alan-agius4 alan-agius4 added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 28, 2025
@alan-agius4
Copy link
Collaborator

@devversion, this does not merge cleanly in the patch branch.

@devversion devversion force-pushed the build-angular branch 2 times, most recently from 3f724f3 to bd820c2 Compare January 28, 2025 19:46
@alan-agius4 alan-agius4 added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Jan 28, 2025
Migrates the `@angular-devkit/build-angular` tests to `rules_js`. This
was a rather larger undertaking as the tests were very reliant on e.g.
the directory structure or specific node module layout; so some changes
were needed.

- the Sass files include a much larger file header now. That is because
  the npm Sass files have much larger paths, given being inside a
  symlinked pnpm store directory. E.g.

  ```

/*!**********************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************!*\
  !*** css ../../../../../node_modules/.aspect_rules_js/[email protected]_webpack_5.97.1/node_modules/css-loader/dist/cjs.js??ruleSet[1].rules[4].rules[0].oneOf[0].use[1]!../../../../../node_modules/.aspect_rules_js/[email protected]_1462687623/node_modules/postcss-loader/dist/cjs.js??ruleSet[1].rules[4].rules[0].oneOf[0].use[2]!./src/test-style-a.css?ngGlobalStyle ***!
  \**********************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************/
.test-a {color: red}
  ```

- Similarly to above, hashed chunk files can change given different
  paths of e.g. Webpack, or external sources.

- Tests for verifying the lazy module chunks may enable
  `preserveSymlinks` just to make the chunk names shorter and easier to
  verify, avoiding truncatd super long paths to the e.g. pnpm stores
  again.

- the ngsw-worker.js file cannot be copied using `copyFile` as that
  results in permissions being copied as well. In Bazel, now that
  the npm files are properly captured, are readonly, so subsequent
  builds (e.g. the watch tests) will fail to copy/override the file
  again! Reading and writing the file consistently seems appropriate.

- Tests relying on puppeteer and webdriver-manager worked in the past,
  by accident, because postinstall scripts (from e.g. puppeteer) were
  able to modify content of other packages (e.g. the puppeteer-core
  cache of browsers then). This does not work with `rules_js` anymore,
  so we need to keep the cache local to the puppeteer postinstall
  script. This requires a little trickery right now to ensure resolution
  of the browsers at runtime works..

- server tests did miss the `node` types to be explicitly listed (as
  they would be in a fresh project), and this caused failures. Likely
  because we no longer patch resolution.

- avoid npm-module style imports from tests within the same package.
  This is not allowed with `rules_js` and also is inconsistent.
@alan-agius4 alan-agius4 added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jan 29, 2025
@alan-agius4 alan-agius4 merged commit 832bfff into angular:main Jan 29, 2025
29 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action: merge The PR is ready for merge by the caretaker area: build & ci Related the build and CI infrastructure of the project target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants