Skip to content

Conversation

jkrems
Copy link
Contributor

@jkrems jkrems commented Nov 5, 2024

Generates unique names for each test entry point and also creates a nice & consistent name for the main ("init test bed") chunk. All specs are now spec-*.js files.

Fixes #28756

P.S.: Current based on #28795, will rebase post-merge.

@jkrems jkrems changed the title Jk fix karma multi spec fix(@angular-devkit/build-angular): handle basename collisions Nov 5, 2024
@jkrems jkrems force-pushed the jk-fix-karma-multi-spec branch from b8d35eb to 7916cb8 Compare November 5, 2024 22:53
@jkrems jkrems marked this pull request as ready for review November 5, 2024 22:57
@jkrems jkrems added action: review The PR is still awaiting reviews from at least one requested reviewer target: rc This PR is targeted for the next release-candidate labels Nov 5, 2024
@jkrems jkrems requested a review from clydin November 5, 2024 22:58
@jkrems jkrems force-pushed the jk-fix-karma-multi-spec branch from 7916cb8 to 5ca8cb8 Compare November 6, 2024 00:00
@jkrems jkrems requested a review from dgp1130 November 6, 2024 00:01
@jkrems
Copy link
Contributor Author

jkrems commented Nov 6, 2024

Adding @dgp1130 as another reviewer in case he has thoughts on the changes to the entryPoints option (based on Chat discussion with @clydin).

Copy link
Collaborator

@dgp1130 dgp1130 left a comment

Choose a reason for hiding this comment

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

The use of a Map to explicitly define output entry points seems generally reasonable. But I'm wondering if we should allow Set at all? I think this was already an issue for the Jest and WTR builders which I hadn't gotten to fixing yet and it makes me think we should never use the Set case as its implemented today or we'll potentially run into the same bug. I'm thinking we should either:

  1. Remove Set from the parameter type altogether and require a Map. All callers need to manually construct this Map.
  2. Update normalizeEntryPoints to find and deduplicate basenames in the set (foo.spec.js becomes foo.spec-2.js) so the caller doesn't need to worry about this.

I'd probably lean towards 2., given that Jest and WTR are likely to duplicate the work done in the Karma builder, and if we go with 2. they likely wouldn't need to worry about this given that none of the test runners really care about the output file name too much.

@jkrems
Copy link
Contributor Author

jkrems commented Nov 6, 2024

A potential issue with 2 is that the test runners (or, at least Karma) need to be able to find the entrypoints afterwards to load them. Which makes me lean towards refactoring the Jest/WTR builders to use the Map as well.

Either way, happy to do a potential follow-up if necessary to ensure that we only have one or the other at the end of this. Agreed that we should pick one data structure, not both.

Copy link
Collaborator

@dgp1130 dgp1130 left a comment

Choose a reason for hiding this comment

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

A potential issue with 2 is that the test runners (or, at least Karma) need to be able to find the entrypoints afterwards to load them. Which makes me lean towards refactoring the Jest/WTR builders to use the Map as well.

I did have a similar thought, but I feel it's manageable if we either maintain the relevant extension (.spec.js), though that might be a little tricky without codifying that particular extension into application builder, or we could potentially dump these into a subdirectory like dist/entry-points/... where the exact file name doesn't matter because all the entry points are there instead of dist/chunk-*.js. Not sure exactly which of these would be easier, but it's a potential option.

I'm fine to tackle that as a follow up, it would definitely be great to have a common solution for all three test runners.

@jkrems
Copy link
Contributor Author

jkrems commented Nov 6, 2024

I'm fine to tackle that as a follow up, it would definitely be great to have a common solution for all three test runners.

FWIW, that is a general theme for the application builder support for Karma. I've been lifting code (e.g. writeTestOutputs) from the other runners and it would likely be nice to refactor to make sure we don't have unnecessary duplication.

@jkrems jkrems force-pushed the jk-fix-karma-multi-spec branch from 5ca8cb8 to 4f8872b Compare November 6, 2024 14:49
@jkrems jkrems force-pushed the jk-fix-karma-multi-spec branch from 4f8872b to 18d0cce Compare November 6, 2024 14:54
@jkrems jkrems 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 Nov 6, 2024
@jkrems jkrems merged commit 2c9904e into angular:main Nov 6, 2024
32 checks passed
@jkrems
Copy link
Contributor Author

jkrems commented Nov 6, 2024

The changes were merged into the following branches: main, 19.0.x

@jkrems jkrems deleted the jk-fix-karma-multi-spec branch November 6, 2024 15:19
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: @angular-devkit/build-angular target: rc This PR is targeted for the next release-candidate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Karma builder can't handle multiple spec files with same name with "builderMode: application"

3 participants