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

fix(@angular-devkit/build-angular): remove double-watch in karma #28794

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

jkrems
Copy link
Contributor

@jkrems jkrems commented Nov 5, 2024

The Karma file watching was racing with the file writes done by the application builder. Since we already tell Karma when to reun via .refeshFiles(), disabling Karma's own file watcher should make things more reliable.

This allows removing a weird special-case in the test case and removes the noisy "File chaned" logs generated by Karma.

Fixes #28755

@jkrems jkrems added the target: rc This PR is targeted for the next release-candidate label Nov 5, 2024
@jkrems jkrems changed the title Address known karma w/ application builder issues in v19 rc.0 fix(@angular-devkit/build-angular): remove double-watch in karma Nov 5, 2024
@jkrems jkrems marked this pull request as ready for review November 5, 2024 16:42
@jkrems jkrems requested a review from clydin November 5, 2024 16:42
@jkrems jkrems added the action: review The PR is still awaiting reviews from at least one requested reviewer label Nov 5, 2024
@jkrems
Copy link
Contributor Author

jkrems commented Nov 5, 2024

Increased the debounce window in the test which shouldn't fundamentally slow the test down. The race between Karma starting to run, the watching build, and the browser running tests may not be worth fixing since it likely requires larger refactoring. Which may end up ugly unless we change Karma as well (and I'm pretty sure we don't want to do that at this point).

Boo. Need to look more into this, it still doesn't appear to be passing consistently on CI.

The Karma file watching was racing with the file writes done by the
application builder. Since we already tell Karma when to reun via
`.refeshFiles()`, disabling Karma's own file watcher should make
things more reliable.

This allows removing a weird special-case in the test case and
removes the noisy "File chaned" logs generated by Karma.

Fixes angular#28755
@jkrems
Copy link
Contributor Author

jkrems commented Nov 5, 2024

The end result is a bit more complex now, mostly to remove a race condition that caused the test instability:

  1. The initial build completes.
  2. During the initial watching build, a file is modified.
  3. The file change is never picked up - there are no further builds and the test cannot make progress.

This is likely not a super common case for real users but correctness-wise, it's likely better to set up the file watching at the very beginning. Doing so requires reusing the same buildApplicationInternal call for the entire test run which makes for ugly code (manual iterator loop instead of a neat for await ... of).

P.S.: Looks like the tests are passing now. Requested re-review because the majority of changed lines is now different than what was originally reviewed.

@jkrems jkrems requested a review from clydin November 5, 2024 19:46
@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 5, 2024
@jkrems jkrems merged commit faabbbf into angular:main Nov 5, 2024
32 checks passed
@jkrems
Copy link
Contributor Author

jkrems commented Nov 5, 2024

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

@jkrems jkrems deleted the jk-fix-karma-rc0 branch November 5, 2024 22:36
@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 6, 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 logs a lot of file changes with "builderMode: application"
2 participants