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(testing): update test regex for 'e2e.ts' #3277

Merged
merged 2 commits into from
Mar 10, 2022

Conversation

rwaskiewicz
Copy link
Contributor

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally and any changes were pushed
  • Unit tests (npm test) were run locally and passed
  • E2E Tests (npm run test.karma.prod) were run locally and passed
  • Prettier (npm run prettier) was run locally and passed

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

c6fda39 created a regression where files named e2e.ts would not be picked up correctly by the test runner

GitHub Issue Number: #3275

What is the new behavior?

this commit fixes an edge case where unit tests were not being properly
detected following c6fda39

the ionic framework has tests named e2e.ts that we must account for in
this regex. because jest doesn't error when no tests are found, we
missed this in the original implementation.

tests of name 'spec.ts' and 'test.ts' (or any other acceptable
extension) are considered unit tests, and tests of the name 'e2e.ts' are
considered end-to-end tests

Does this introduce a breaking change?

  • Yes
  • No

Testing

I pulled this branch, built this branch, and generated a tarball: npm ci && npm run build && npm pack

Within Ionic Core, I installed my tarball in ionic-framework/core and ran two tests:

  1. Tested the original reproduction command: npm run test src/components/modal/test/basic to verify tests are run
  2. Ran the entire test suite, taking note of the number of tests that are run with npm t -- --no-cache:
    Screen Shot 2022-03-10 at 4 05 55 PM

I then installed Stencil v2.14.0 (prior to the regression) and ran the entire test suite to verify the counts aligned:
Screen Shot 2022-03-10 at 4 00 47 PM

(this doesn't to a test-for-test comparison, but it does give us a high degree of confidence that the fix works when the first test case is taken into account).

I finally installed Stencil v2.14.1 (which has the regression) to observe some tests are omitted:
Screen Shot 2022-03-10 at 4 08 55 PM

Other information

Related to #3276, but at the time of this writing am not 100% sure
this solves the that issue

this commit fixes an edge case where unit tests were not being properly
detected following c6fda39

the ionic framework has tests named `e2e.ts` that we must account for in
this regex. because jest doesn't error when no tests are found, we
missed this in the original implementation.

tests of name 'spec.ts' and 'test.ts' (or any other acceptable
extension) are considered unit tests, and tests of the name 'e2e.ts' are
considered end-to-end tests
@rwaskiewicz rwaskiewicz requested a review from a team March 10, 2022 21:09
@rwaskiewicz rwaskiewicz merged commit cf42114 into main Mar 10, 2022
@rwaskiewicz rwaskiewicz deleted the rwaskiewicz/fix-unit-test-regex branch March 10, 2022 22:19
@onhate
Copy link

onhate commented Mar 14, 2022

@rwaskiewicz @ltm on this release jest is trying to load .spec.ts.snap files, I see the regex is missing the $ in the end

 FAIL  src/components/pulpo-video/__snapshots__/pulpo-video.spec.ts.snap
  ● Test suite failed to run

    Your test suite must contain at least one test.

@rwaskiewicz
Copy link
Contributor Author

@onhate good catch! The issue you mentioned will be resolved by #3282

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants