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

migrate comparison tests to Jest snapshot tests #1161

Closed
wants to merge 69 commits into from
Closed

migrate comparison tests to Jest snapshot tests #1161

wants to merge 69 commits into from

Conversation

g-plane
Copy link
Contributor

@g-plane g-plane commented Jul 30, 2020

This PR is still in progress.

This PR migrates comparison tests to Jest snapshot tests. The advantages for this change are:

  • Better error messages when tests are failed, even snapshots aren't matched.
  • All tests are run in the same process and this should be faster. This is different from current comparison tests. The workflow of current comparison tests is to spawn a Mocha process for each test, which is slow.

However, the main issue of this migration is CI on Windows. Snapshots run on Linux is stable, while running on Windows is fragile with unknown reasons, and some tests may fail on Windows.

Some questions about this PR:

  • Is the direction of this PR right?
  • Is there a better way to solve the problem of running tests on Windows?

@johnnyreilly
Copy link
Member

Cool - I'm on my holidays right now but will take a look soon!

@johnnyreilly
Copy link
Member

Wow this is a giant PR! Kudos for getting going with it.

However, the main issue of this migration is CI on Windows. Snapshots run on Linux is stable, while running on Windows is fragile with unknown reasons, and some tests may fail on Windows.

This is a problem - if we the test pack is unreliable on Windows then it's a bit of an issue.

When it comes to the expectedOutput-X.X versions I should really have been more disciplined. We only run the latest version of TypeScript against the comparison test pack. I don't have any plans to change that. Getting rid of all theexpectedOutput-X.X versions apart from expectedOutput-3.9 is probably a good idea.

Just to help me review this a bit better, which are files I should particularly take a look at (given there's so many that are snapshots)?

@g-plane
Copy link
Contributor Author

g-plane commented Jul 31, 2020

I've migrated each case by each commit. For example, the es6 case are made in make snapshot: es6 commit. Every commit whose message starts with "make snapshot" indicates this commit is for a specified case.

And how to review each test case? Well, the output are saved to __snapsots__/test.ts.snap, which is different from the current comparison test. Open the .snap file, you'll find several snapshots, and each one comes with a title. For example, build means a webpack compliation with TS type checking, while transpile only not. And, bundle means the webpack asset output (bundle.js), and stats means webpack stats. You can compare them by using Git diff.

@johnnyreilly
Copy link
Member

I'm guessing the Windows tests are still flaky?

@g-plane
Copy link
Contributor Author

g-plane commented Aug 10, 2020

Windows tests are more stable than before except the "basic" case, because I've increased the test timeout and limited the workers to "1".

@johnnyreilly
Copy link
Member

But it looks like the windows tests are still failing?

@g-plane
Copy link
Contributor Author

g-plane commented Aug 10, 2020

It only failed on the test case "basic", and you can view the CI log output to check it.

@johnnyreilly
Copy link
Member

johnnyreilly commented Aug 14, 2020

It looks like there's other failures too?

FAIL  test/comparison-tests/importsWatch/test.ts (306.676 s)

Would it be possible to have a re-run mechanism in place for tests - allow them a number of attempts to pass to allow for flakiness?

Or do all the tests that fail on windows reliably fail? (And if so - "why?" Is the question I guess)

@g-plane
Copy link
Contributor Author

g-plane commented Aug 14, 2020

Actually, it has "re-run" mechanism:

https://github.com/g-plane/ts-loader/blob/2aaffbc2116f6079383624c3384fc6b4b867e7f7/test/comparison-tests/declarationWatch/test.ts#L5

All the tests with webpack watch mode will be allowed to be re-run.

As for the failures on Windows, it occurred sometimes, not always. And those failures are only happened with webpack watch mode. Those tests which run a single build (not watching any files, just build them once) are always stable, even on Windows.

@johnnyreilly
Copy link
Member

As for the failures on Windows, it occurred sometimes, not always.

Should we just be repeating a sufficiently large number of times on Windows?

@g-plane
Copy link
Contributor Author

g-plane commented Aug 14, 2020

Maybe we can try.

@johnnyreilly
Copy link
Member

Arghhhh!!!!!

@g-plane
Copy link
Contributor Author

g-plane commented Oct 17, 2020

I'm still not able to solve this Windows problem. I may close this PR.

@johnnyreilly
Copy link
Member

It's a real shame - thank you for your efforts. I do appreciate your work.

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.

None yet

2 participants