Skip to content

Conversation

@gnbm
Copy link

@gnbm gnbm commented Oct 20, 2025

What is the current behavior?

  • Standardize on Jest across the repo, remove Karma, and split CI jobs for faster signal and isolation.

GitHub Issue Number: N/A

What is the new behavior?

  • Replaced Karma in test/bundler with Jest:
    • Added test/bundler/jest-dom-utils.ts (jsdom loader that executes built module scripts and waits for appload/componentOnReady).
    • Converted test/bundler/vite-bundle-test/vite-bundle.spec.ts to Jest and pointed it to the built index.html.
    • Deprecated local Karma files: test/bundler/karma.config.ts, karma-stencil-utils.ts, and updated test/bundler/package.json.
  • Added bundler sub-Jest config: test/bundler/jest.config.js.
  • Added scripts:
    • test.bundler`: run bundler Jest with sub-config.
    • ci.test: run full repo Jest + bundler sub-config.

CI:

  • New workflow .github/workflows/ci.yml with separate jobs:
    • Core Jest (full test suite).
    • Bundler Jest (sub-config).

Documentation

N/A

Does this introduce a breaking change?

  • Yes
  • No

Testing

  • Local run:
    • Build as usual, then:
      • Core: npm run test.jest
      • Bundler: npm run test.bundler

Other information

  • Impact:

    • No runtime/build changes; only tests and CI.
    • Bundler test now runs under Jest; CI can call npm run ci.test (or each job separately).
  • Notes: The core suite is green. Occasional “worker failed to exit gracefully” warnings persist but are benign.

@gnbm gnbm added the dependencies Pull requests that update a dependency file label Oct 20, 2025
@gnbm gnbm changed the title chore(test): migrate Angular example from Karma to Jest; add CI jobs and mappings chore(test): migrate from Karma to Jest and add CI jobs and mappings Oct 20, 2025
@gnbm gnbm marked this pull request as ready for review October 20, 2025 21:56
@gnbm gnbm requested a review from a team as a code owner October 20, 2025 21:56
Copy link
Contributor

@johnjenkins johnjenkins left a comment

Choose a reason for hiding this comment

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

Love the pipeline icons 😄
Just one question

};
window.addEventListener('appload', onAppLoad);
// if app already loaded synchronously, resolve on next tick
setTimeout(() => resolve(), 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

how does this 'know' that the app loaded synchronously here sorry?
Won't this just resolve every time after a tick?

Copy link
Author

@gnbm gnbm Oct 22, 2025

Choose a reason for hiding this comment

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

Great question! This is actually a bug in the code. You're absolutely right - the setTimeout(() => resolve(), 0) will always resolve after one tick, regardless of whether the app actually loaded or not.
Will look into this asap

@johnjenkins
Copy link
Contributor

johnjenkins commented Oct 22, 2025

@gnbm - just to clarify ... these tests used to run in an actual browser and now they run in ... jsdom(?)
Is that correct / is that what we want? (I may just be confused 😅 )

gnbm and others added 3 commits October 22, 2025 13:17
The previous implementation had a setTimeout with 0ms that would always resolve
immediately, making the 'appload' event listener effectively useless. This could
cause tests to continue before the app was actually ready.

This fix:
- Adds a resolved flag to prevent double resolution
- Changes timeout from 0ms to 5000ms (reasonable fallback)
- Only resolves via timeout if the event hasn't fired
- Adds warning message when falling back to timeout
- Properly cleans up event listener in both paths
@gnbm
Copy link
Author

gnbm commented Oct 22, 2025

just to clarify ... these tests used to run in an actual browser and now they run in ... jsdom(?)
Is that correct / is that what we want? (I may just be confused 😅 )

Now that I'm thinking better about this, I probably should have migrated to Playwright instead, since with this approach, even though the tests would run faster, we'll lose a lot of things about real browsers. I'll close the PR and look into this from a different perspective. Thank you for the review and for raising all of this, since I still don't have enough context and was just trying to start contributing with some AI help to accelerate the process.

@gnbm gnbm closed this Oct 22, 2025
@johnjenkins
Copy link
Contributor

Now that I'm thinking better about this, I probably should have migrated to Playwright instead, since with this approach, even though the tests would run faster, we'll lose a lot of things about real browsers. I'll close the PR and look into this from a different perspective. Thank you for the review and for raising all of this, since I still don't have enough context and was just trying to start contributing with some AI help to accelerate the process.

Absolutely no problem - appreciate you trying to tidy this up!
We do use jest with a puppeteer / browser runner ... I'm just not too sure how you'd use (or if you should).

@gnbm
Copy link
Author

gnbm commented Oct 22, 2025

Now that I'm thinking better about this, I probably should have migrated to Playwright instead, since with this approach, even though the tests would run faster, we'll lose a lot of things about real browsers. I'll close the PR and look into this from a different perspective. Thank you for the review and for raising all of this, since I still don't have enough context and was just trying to start contributing with some AI help to accelerate the process.

Absolutely no problem - appreciate you trying to tidy this up! We do use jest with a puppeteer / browser runner ... I'm just not too sure how you'd use (or if you should).

Will explore that option first. Thanks for sharing and for your amazing contribution to this great project

@gnbm gnbm deleted the chore/jest-migrate-bundler-and-ci branch October 22, 2025 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants