search

serut / meteor-coverage

Server and client coverage for Meteor
MIT License
54 stars 14 forks source link

Istanbul babel plugin #73

Closed SimonSimCity closed 5 years ago

SimonSimCity commented 6 years ago

Replaced the part of file instrumentation and source maps by the babel plugin babel-plugin-istanbul

Description

This patch replaces the full implementation of instrumentation and source maps by the babel plugin babel-plugin-istanbul.

This change also rendered the configuration for including and excluding files superfluous, which now can be configured in the babel-configuration (see: https://github.com/istanbuljs/babel-plugin-istanbul#ignoring-files)

Motivation and Context

There are several things we'll gain by using the babel plugin instead of a custom implementation:

How Has This Been Tested?

I had to remove most of the tests, since most of them were covering logic that's not needed anymore. I therefore have tested it manually - but only for code-coverage on a project - not when developing a plugin.

I just don't know if it's possible to use babel when running a test of a meteor-package ...

Types of changes

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at ?% when pulling 29f5ed33eb6209e9183846fc7f82fd4f004e8530 on SimonSimCity:istanbul-babel-plugin into 286118b1ee910cdb07180d209c52f0cb0e2e6a3a on serut:master.

serut commented 6 years ago

Yeah the code is much simpler on this PR that what we have on master. Massive work as always ! πŸ₯‡ I don't think too there is a babel option for packages... But maybe we can use the empty meteor app that I create on test to setup babel . The failing test is testing the size of the package on the client side, if it's instrumented it should be a lot more heavier that what we get (~5500caracters).

SimonSimCity commented 6 years ago

I can confirm that the code coverage is way more accurate with the babel plugin (specially if you heavily use ECMAScript+ features) and it will most-likely therefore also take down the result of your coverage reports - down to their actual value πŸ˜„

serut commented 6 years ago

Wonderful ! It looks really nice ! We will ship this in a v3 release when it will be tested and some doc rewrited. We need to find a solution for packages too, if we just need an empty meteor app that's fine...
Can you publish your workspace with your sample app in a fork of meteor-coverage-app-example ? We need to check if Travis can export that coverage report before merging this PR anyway, if you can publish it, I would save me tons of install.

codecov-io commented 5 years ago

Codecov Report

Merging #73 into master will decrease coverage by 9.57%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #73      +/-   ##
==========================================
- Coverage   69.71%   60.14%   -9.58%     
==========================================
  Files          21        1      -20     
  Lines         743      680      -63     
==========================================
- Hits          518      409     -109     
- Misses        225      271      +46
Impacted Files Coverage Ξ”
...es/meteor-coverage/server/report/report-service.js
...teor-coverage/server/report/report-text-summary.js
packages/meteor-coverage/client/methods.js
packages/meteor-coverage/server/boot.js
packages/meteor-coverage/server/index.js
...ages/meteor-coverage/server/report/report-remap.js
packages/meteor-coverage/server/handlers.js
...kages/meteor-coverage/server/report/report-html.js
packages/meteor-coverage/server/services/core.js
...es/meteor-coverage/server/report/report-generic.js
... and 12 more

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 286118b...d508e97. Read the comment docs.

SimonSimCity commented 5 years ago

It has been a quite long time, but now I finally found some time to fork it and update the files there.

I can hereby prove that the coverage of project-files work and is quite stable. The coverage of meteor-packages does not work as of now, because I have no idea how to add the babel-plugin to the list of babel-plugins which are ran over the generated packages - this is something I'm still investigating on ...

Here's the link to the repository: https://github.com/SimonSimCity/meteor-coverage-app-exemple

Please ping me if you know anything more - specially anything about the babel-compiling of meteor-plugins.

Btw: I had to update the project to Meteor v1.7 because the package-tests didn't run on 1.6 for some reason - didn't investigate any longer because it only pointed me to a compatibility-issue which was introduced in babel-beta >55, but I had it installed in beta-45.

Running the tests of meteor-coverage now fails, but it's only the reports. I guess there's still something I need to fix there.

serut commented 5 years ago

Ooooh ! I will need to get some time to investigate this and merge everything !

SimonSimCity commented 5 years ago

Just to add some hints ...

serut commented 5 years ago

Ohhh ! I get some time to test it and this is working very great so far ! There is nothing to fix on this PR, even if we loose the support for packages, the quality of the instrumentation is much better and makes the change worth enough. And this is quite intuitive so the tutorial won't be complicated.

SimonSimCity commented 5 years ago

This will also require a completely different setup for the coming version of meteor-coverage ... and I wonder what a good way of introducing this would be. The meteortesting:meteor-mocha has some minimal configuration (https://github.com/meteortesting/meteor-mocha#run-with-code-coverage) and

I'm also working on a rewrite of the chapter of testing in the meteor guide (https://forums.meteor.com/t/which-testing-frameworks-are-popular-today-end-2018/46480) where this package also will be mentioned in connection with the other testing frameworks it works together with.

I'm glad we got this great improvement finally landed and I'll continue searching for a way to get this work for meteor-packages - don't know how active but I'll definitively give it a second try.

serut commented 5 years ago

Yes that's not yet finished! One good point is the new application setup, that's so easy if we look at this tiny commit.

I will use that app in meteor-coverage-app-exemple to create the new minimal setup. If you think we can add more, feel free to submit a PR or an issue against that repo.

There is a lot of work to rewrite the readme of this repo, but I have some ideas, I just some spare time !

As you say, there is much more to do on the meteor testing guide. The base is good but some dependencies are outdated and there is details completly missing when it goes to real world. They should consider the reader as an open source app creator that wants to runs its tests on a free CI platform. It should be done under 15 mins, without jumping from section to other section.

About package coverage, I have two solutions in mind :

serut commented 5 years ago

I've published a new package meteor-packages-coverage that is a fork of meteor-coverage when it was covering files. At least we have a solid solution for packages coverage.

And as you can see, meteor-coverage tests have been fixed on CI platforms Travis and Circle.ci. Everytime I upgrade Meteor there is a new bug, this time that was chai not found on server tests. Full bullshit β˜•οΈ

Just need to rewrite the README now ! ;)

SimonSimCity commented 5 years ago

You've added the package meteor-package-coverage as a folder to this repository ... I hope you didn't also publish it alongside 😟

serut commented 5 years ago

There is no auto import on packages so everything imported is listed in the package.js. And meteor-coverage is not published yet as the readme still needs some update. ;)

SimonSimCity commented 5 years ago

Just make sure you remove the files before building a version of this package.

Even though these files are not imported, they might be published alongside with the rest of the files of the plugin. meteor-collection-hooks for example got this wrong lately - just see their issue https://github.com/matb33/meteor-collection-hooks/issues/246 ...

serut commented 5 years ago

Ohh I didn't expect that... Thanks for the tips ! I will move to another repository