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

Istanbul babel plugin #73

Merged
merged 4 commits into from
Nov 10, 2018
Merged

Conversation

SimonSimCity
Copy link
Contributor

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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@SimonSimCity SimonSimCity mentioned this pull request May 10, 2018
3 tasks
@coveralls
Copy link

Coverage Status

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

@serut
Copy link
Owner

serut commented May 10, 2018

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
Copy link
Contributor Author

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
Copy link
Owner

serut commented May 10, 2018

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
Copy link

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
Copy link
Contributor Author

SimonSimCity commented Sep 19, 2018

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
Copy link
Owner

serut commented Sep 19, 2018

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

@SimonSimCity
Copy link
Contributor Author

Just to add some hints ...

  • Maybe we should also propose an update for https://github.com/meteor/guide/blob/master/content/testing.md when updating the readme here. It should be mentioned in the guide as code-coverage is quite critical for developing professional business-applications.
  • My repo has two interesting branches: istanbul-babel-plugin, showing off how it works with application code with the least changes to your code. istanbul-babel-meteor-plugins is a work-in-progress for getting coverage for meteor-packages. Doesn't work as of now, but some changes there should definitively make it into a guide. E.g. the npm-commands and getting rid of phantomjs.
  • Please follow https://forums.meteor.com/t/how-to-load-babel-plugins-for-a-meteor-package/45717 to get updated when and how we can fix the coverage for meteor-packages.

@serut
Copy link
Owner

serut commented Nov 10, 2018

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.

@serut serut merged commit d4dc9f4 into serut:master Nov 10, 2018
@SimonSimCity
Copy link
Contributor Author

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
Copy link
Owner

serut commented Nov 15, 2018

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 :

  • either we can figure it out how meteor bundles when we run test-packages, and what differs from the test one. I don't know in normal mode if packages are read by babel or directly append like that in the codebase, but I think it does not worth to spend hours on that. Meteor goes in to the NPM hub, packages are king of outdated.
  • either we create another package that uses the algorithm we removed on this PR to allow packages to be manually instrumented like it worked previously. It can be easily done if that's possible.

@serut
Copy link
Owner

serut commented Nov 20, 2018

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
Copy link
Contributor Author

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

@serut
Copy link
Owner

serut commented Nov 20, 2018

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
Copy link
Contributor Author

SimonSimCity commented Nov 20, 2018

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 Meteor-Community-Packages/meteor-collection-hooks#246 ...

@serut
Copy link
Owner

serut commented Nov 20, 2018

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

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

4 participants