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

Implement test coverage feature #390

Merged
merged 1 commit into from
Feb 5, 2019

Conversation

parostatkiem-zz
Copy link
Contributor

@parostatkiem-zz parostatkiem-zz commented Jan 30, 2019

Description

⚠️"Why do I see an alpha package?!"

Istanbul hasn't been officially updated for 2 years since 0.4.5 version came out. That version (the newest stable one) uses babel-core 6.x.x and LuigiCore uses @babel/core ^7.0.0.
Somehow it just can't run together in harmony, furthermore having 2 Babels would be confusing (at least). I've searched the whole Internet and everyone says the only solution is including an alpha version of Istanbul which works really well (they say) and uses @babel/core.

"What about test:watch?"

It works as it did before. The report is generated after you end the watch process (ctrl+c).
No, there's no other solution. Istanbul collects data from the output of Mocha's test process. An output is generated at process' exit.

Although final changes are really small, this wasn't an easy task. I've tried everything I could to avoid alpha version of Istanbul + there were many more complications.

Changes proposed in this pull request:

  • Use Istanbul lib to generate test coverage report
  • ...
  • ...

Related issue(s)

@dariadomagala-sap
Copy link
Contributor

dariadomagala-sap commented Jan 31, 2019

Is there anything else other than Istanbul? Maybe there is a different package that is stable and properly tested?

https://github.com/istanbuljs/nyc ?

@dariadomagala-sap dariadomagala-sap self-assigned this Jan 31, 2019
@parostatkiem-zz
Copy link
Contributor Author

parostatkiem-zz commented Jan 31, 2019

Didn't work, the other Istanbul extension (forgot the name) also didn't work.
Also tried Solution A from this. Didn't work.

@y-kkamil y-kkamil self-assigned this Feb 4, 2019
@y-kkamil
Copy link

y-kkamil commented Feb 4, 2019

I have managed to run nyc as a coverage reporter. It gives different output than your solution, though. I think we should investigate this further - where this difference comes from, which one is correct, and is there any added value for using nyc over old istanbul.

@parostatkiem-zz
Copy link
Contributor Author

@y-kkamil Looks like NYC is just a command line overlay for Istanbul so they seem to have common roots. I've got no idea what causes the coverage differences then 😆

IMO we can stick to my solution. As I said on daily, in the worst case that the alpha package fails, no customer/Luigi user will be harmed. We just won't be able to see test coverage, not a big deal...
Furthermore many people reported this package to be stable.

@parostatkiem-zz parostatkiem-zz merged commit fba299a into SAP:master Feb 5, 2019
@parostatkiem-zz parostatkiem-zz deleted the unit-tests-coverage branch February 5, 2019 10:39
stanleychh pushed a commit to stanleychh/luigi that referenced this pull request Dec 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants