test: Add jest and first tests#114
Conversation
brody2consult
left a comment
brody2consult
left a comment
There was a problem hiding this comment.
This is really nice in general. I left a couple comments.
In general, I am wondering whether or not we need to keep all comments in the generated jest.config.js file. The positive is that it may be easier for us to select some more Jest options in the future.
OTOH, I do actually recall running Jest with no extra config file in some other projects.
| const XML = ` | ||
| <pdf2xml producer="poppler" version="0.26.5"> |
There was a problem hiding this comment.
As a nit, I think this will give us a string with an extra blank line. I think it would be ideal to use something like dedent or outdent. But probably not worth changing in this PR.
brody2consult
left a comment
brody2consult
left a comment
There was a problem hiding this comment.
Approved with a few nits that I think are not worth fixing in this PR:
- I generally like to avoid adding TODO / FIXME items.
- I think it would be ideal to move the executable example test for issue #93 into a separate PR. I would like to remove the fixes message when merging this PR, and get the answer to #93 documented someday.
- I left a comment that a template string will get an extra blank line.
|
I totally agree on your comments. Maybe I should rather have put the "Todo/Fixme comments into this PR instead of in the code, since my intention was not to land them, but to get feedback on them. |
Got it now, that was not clear to me when I reviewed the changes. I did want to get this PR finished and merged pretty quickly in order to unblock other progress, considering that I think test coverage is way more important than dealing with some other loose ends. That is what I tried to say by my nits "not worth fixing in this PR". In case we do expect any loose ends in a PR, I think it would be a nice bonus to raise an issue. But comments like TBD, TODO, FIXME are fine with me as well if they make sense. Thanks again for your contributions. |
2 participants
as discussed in #72 and #112 so we can get started using jest for new tests.
npm run testrun everything there isnpm run jestjust runs the jest tests (test/*.test.js)(The tests have been picked from cases that stryker identified in my run in #112 .)