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

Transition to Jest, Increase Languages Tested, and Fix Bug with Sub-Languages #10

Merged
merged 9 commits into from
Nov 25, 2017

Conversation

keplersj
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Nov 23, 2017

Codecov Report

Merging #10 into master will increase coverage by 1.44%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #10      +/-   ##
=========================================
+ Coverage   63.26%   64.7%   +1.44%     
=========================================
  Files           2       2              
  Lines          49      51       +2     
  Branches        9      10       +1     
=========================================
+ Hits           31      33       +2     
  Misses         18      18
Impacted Files Coverage Δ
src/index.ts 89.28% <100%> (+0.82%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d6d026...de2b227. Read the comment docs.

@keplersj keplersj changed the title WIP: Transition to Jest Transition to Jest, Increase Languages Tested, and Fix Bug with Sub-Languages Nov 23, 2017
@felixfbecker
Copy link
Owner

Why the change to jest?

@keplersj
Copy link
Contributor Author

Primarily to make use of Snapshots.

@felixfbecker
Copy link
Owner

Could you explain that feature or link me to docs? I am always a bit reluctant to switch to different frameworks without evaluating the pros/cons.

@keplersj
Copy link
Contributor Author

No worries. Totally get it. Currently this repo takes an input and tracks it against an expected output. Snapshots simply automate the tracking of the expected output by placing it in external snapshot files. This is super helpful for dynamically generated tests, which this PR also does by looping through fixtures and testing against them.

For more on Snapshot testing here's a talk from React Conf 2017 about it, and the documentation on Snapshot Testing

@felixfbecker
Copy link
Owner

felixfbecker commented Nov 24, 2017

I generally like to compose unopinionated libraries, and jest looks very opinionated on first sight (all the file naming conventions, included assertions and coverage tools, ...) but this sounds like a useful feature that would require boilerplate to replicate with mocha.

.travis.yml Outdated
@@ -16,11 +16,9 @@ install:

script:
- npm run lint
- npm run build
Copy link
Owner

Choose a reason for hiding this comment

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

I would still like to run this in CI to see if the compilation has any errors.
Then of course the compilation that jest does is a bit redundant, is it possible to just run jet from the output JS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To work with TypeScript I set up Jest to use a package called ts-jest. Running tsc before Jest wouldn't have any effect on the tests. Happy to add it back

Copy link
Owner

Choose a reason for hiding this comment

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

So it's not possible to just run the generated .js files with jest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's possible. Using ts-jest ensures that code coverage reports produced are accurate. But it's not necessary jestjs/jest#336 (comment) https://github.com/codecov/example-typescript/issues/7

Copy link
Owner

Choose a reason for hiding this comment

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

Okay, that's a priority of course. Compilation should be fast so not worried that much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So what were you thinking on this one? Stick with ts-jest or, compile then test like in the SourceGraph example?

Copy link
Owner

@felixfbecker felixfbecker Nov 25, 2017

Choose a reason for hiding this comment

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

We can use ts-jest. What do you mean with Sourcegraph example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Err. My bad. The Codecov example. https://github.com/codecov/example-typescript

src/test/test.ts Outdated
} else {
console.log(highlighted + '\n')

if (process.env.VERBOSE) {
Copy link
Owner

Choose a reason for hiding this comment

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

Can you set this in CI? In opposite to the previous assertion way, the snapshot files contain the raw ANSI codes, which is impossible to code review. Printing the coloured output in CI is a good and easy way to verify that the changes the snapshots were correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course!

Copy link
Contributor Author

Choose a reason for hiding this comment

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


after_success:
- nyc report --reporter=json
Copy link
Owner

Choose a reason for hiding this comment

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

jest seems to always generate JSON, lcov and clover format, which are then all three uploaded to codecov. I am a bit worried that if one format is better than the other (does lcov support sub-line coverage?) we get the lowest common denominator of features as codecov merges the coverage. Is there a way to only tell jest to generate JSON?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do using this configuration option

Copy link
Owner

Choose a reason for hiding this comment

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

Cool, lets set that to text-summary and json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@felixfbecker felixfbecker merged commit d798e0f into felixfbecker:master Nov 25, 2017
@keplersj
Copy link
Contributor Author

Woot! 🎉 Thank you!

@felixfbecker
Copy link
Owner

Thanks for your contributions! You turned this into a really solid module

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