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

feat: move from mocha to jest #572

Merged
merged 28 commits into from
Mar 16, 2022
Merged

feat: move from mocha to jest #572

merged 28 commits into from
Mar 16, 2022

Conversation

Kreeg
Copy link
Member

@Kreeg Kreeg commented Feb 23, 2022

Pros:

  • complete package
  • integrated parallel multi-worker tests runner
  • integrated code coverage (and more accurate than c8, can be confirmed by running npm run coverage:jest and npm run coverage:jest:native). Bye c8.
  • integrated stubs, mocks (bye sinon)
  • integrated assertion library (bye should, assert)
  • great and big community with a lot of plugins
  • es modules ready (code transform with babel-jest)
  • integrated watcher jest --watch

Cons:

  • this is a BIG package
  • the first run may took a long time (if the code transformation is included)
  • may need an adjustments with current github actions

TODO:

  • remove unused packages and files
  • configure Jest's coverage to include all files in bin and lib folder, and use the same reporters as before
  • see if you can split the async compile in a separate PR since it's a feature that doesn't really belong to the Jest changes
  • see if the root folder is good practice to keep the jest files
  • remove the constants file since it's pretty much useless and we don't need to test that string IMHO
  • fix exceeding timeout on failed tests

Fixes #445 along the way

@XhmikosR
Copy link
Member

XhmikosR commented Feb 26, 2022

I personally have no interest in this so I'm +0. But if it helps you move easier, I don't mind the switch.

I could also argue about some of the pros you list, but none of us will benefit from this ;)

Bottom line, I don't mind moving to Jest, but this PR is far from ready AFAICT.

EDIT: FYI this is my personal favorite https://github.com/lukeed/uvu which I use when I can to replace mocha.

@Kreeg
Copy link
Member Author

Kreeg commented Feb 27, 2022

Yeah I just wanted to draw your attention to this and to get your opinion. Thanks for reply I'll move forward with this.

@Kreeg Kreeg changed the title feat: moving from mocha to jest feat: moving from mocha to jest (autofix #445) Mar 9, 2022
@Kreeg Kreeg linked an issue Mar 9, 2022 that may be closed by this pull request
@Kreeg Kreeg changed the title feat: moving from mocha to jest (autofix #445) feat: moving from mocha to jest Mar 9, 2022
@Kreeg
Copy link
Member Author

Kreeg commented Mar 12, 2022

JEST

Test Suites: 14 passed, 14 total
Tests:       110 passed, 110 total
Snapshots:   0 total
Time:        27.496 s

MOCHA

 113 passing (47s)

Jest Coverage (native)

All files                     |      85 |    64.68 |   88.05 |   85.17 |

Jest coverage (c8)

All files                     |   81.44 |    77.24 |    90.9 |   81.44

Mocha Coverage (с8)

All files                     |    81.1 |    76.16 |   88.69 |    81.1 |

NOTE: shape.test.jest.js was changed because of mocking the results, and thus getting rid of obsolete tests

package.json Outdated Show resolved Hide resolved
@XhmikosR
Copy link
Member

@Kreeg when you are ready let me know so that I can have a closer look :)

@XhmikosR
Copy link
Member

BTW one things that bothers me on main is that after running each test file, the tmp folder is cleaned. So, if one test suite fails, I have to explicitly run the failed one again so that I can get the updated PNG files from tmp.

I think we should clear the tmp folder via cli or just don't clear it at all.

@Kreeg
Copy link
Member Author

Kreeg commented Mar 12, 2022

@XhmikosR Ok I'll tell you when it is ready
But I am not quite follow you on deleting paths. tmp path is removed before all test are ran, each test has his own directory inside tmp and is cleaned before the tests inside the current test suite.
There is no teardown in which tmp path is deleted so it exists after tests are passed up till the next run

@XhmikosR
Copy link
Member

No worries, we'll check it out again later :)

@Kreeg
Copy link
Member Author

Kreeg commented Mar 13, 2022

@XhmikosR Hey! The first step is done. I kept old test files for visual comparing with the new ones and to have ability to run old tests.
The next step will be removing useless dependencies, removing old tests and renaming new ones to .test.js, and updating scripts in package.json

@Kreeg Kreeg requested a review from XhmikosR March 14, 2022 15:51
@XhmikosR
Copy link
Member

@Kreeg I think these only are left:

  1. see if there are more instances of toStrictEqual and similar with toBe or toEqual when possible (if the remaining instances need to be strictEqual, we are fine :))
  2. maybe split those long lines to expected/input variables (I'll have another look myself later in case we missed something)

They are not blocking of course, it's just nice to have before we land this huge PR :)

I also have a question about the coverage but I'm not sure if it's a misconfiguration issue. I see duplicate files in coverage/ and in coverage/lcov-report/ folders.

Again, great work!

@Kreeg
Copy link
Member Author

Kreeg commented Mar 14, 2022

@XhmikosR

  1. There is no toEqual now, only toStrictEqual for the objects and toBe for the rest.
  2. Can you please specify this lines? The problem is only with the length? This can be easily auto-fixed with prettier ( I know, it's me again with that prettier, but we can turn it on in tests folder for now). If length is not the case (more readability) we can refactor to use expect(input).to...(expected) in this PR.
  3. I faced now issue with the failed tests. They are all fall by timeout. I'll check it. That's critical

I'll check later issue with coverage.

@XhmikosR
Copy link
Member

Re 1., 2. all good now :)

About 3., it's not new it's the same issue we had in the past. 15s might work better.

@Kreeg
Copy link
Member Author

Kreeg commented Mar 16, 2022

@XhmikosR yeah createDiff takes too long. So I increased timeout.

@Kreeg
Copy link
Member Author

Kreeg commented Mar 16, 2022

@XhmikosR so it's ready now

@Kreeg
Copy link
Member Author

Kreeg commented Mar 16, 2022

About coverage folders. The files are not quite the same. I think it's jest "feature". Is it a problem?

@XhmikosR
Copy link
Member

Great, let's land it and we can tweak it later!

  1. more async in tests (and later in core)
  2. see if we can figure out the duplicate coverage issue

@XhmikosR
Copy link
Member

Not a problem, it just confuses me, personally. I mean, which one is the "right" one?

@XhmikosR XhmikosR merged commit 10fa340 into main Mar 16, 2022
@XhmikosR XhmikosR deleted the jest branch March 16, 2022 16:09
@Kreeg
Copy link
Member Author

Kreeg commented Mar 16, 2022

I'm not sure which is correct too. I think they are all correct, but looks different (some files are not present in lcov directory)

@XhmikosR
Copy link
Member

Can you make a branch adding your preferred prettier options but trying to match the current style?

Assuming we have a prettier config file, we should be able to make use of it in the whole codebase and for JS we can use xo + prettier.

@Kreeg
Copy link
Member Author

Kreeg commented Mar 16, 2022

@XhmikosR OK! But I think to match all this styles with prettier is impossible but I'll try

@XhmikosR
Copy link
Member

Yeah, we'll need to adapt some things for sure but it should be doable :)

@XhmikosR XhmikosR changed the title feat: moving from mocha to jest feat: move from mocha to jest Mar 19, 2022
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 refactor tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make each test case individually testable
2 participants