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

Next release #30

Merged
merged 31 commits into from
Sep 21, 2016
Merged

Next release #30

merged 31 commits into from
Sep 21, 2016

Conversation

serut
Copy link
Owner

@serut serut commented Sep 12, 2016

My bad, something is wrong with the default coverage.json file now.
Otherwise, it looks stable so far.

@codecov-io
Copy link

codecov-io commented Sep 12, 2016

Current coverage is 75.96% (diff: 76.01%)

Merging #30 into master will increase coverage by 0.89%

@@             master        #30   diff @@
==========================================
  Files            20         22     +2   
  Lines           658        774   +116   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            494        588    +94   
- Misses          164        186    +22   
  Partials          0          0          

Powered by Codecov. Last update 7075ff7...9bfcda0

"**/.npm/package/node_modules/**",
"**/web.browser/packages/**",
"**/.?*/**",
"**/packages/!(local-test_?*.js)",
Copy link

@cgalvarez cgalvarez Sep 13, 2016

Choose a reason for hiding this comment

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

The glob pattern **/.npm/package/node_modules/** is already catched by **/.?*/**, which excludes any hidden folder, but does not cover node_modules folder in a meteor app (in a meteor app, the used NPM modules are installed at node_modules, not .npm/package/node_modules), and that's why I set it up as **/node_modules/**. I'm developing a meteor app with angular2-meteor, and all the NPM packages are saved to node_modules/**, so the current glob pattern presumably will not catch it (not tested yet, I'm focused on meteor package testing right now).

**/web.browser/packages/** is already catched by **/packages/!(local-test_?*.js), except the last one will exclude the compiled & run test file, which it's needed to traverse the tested file(s) through its source map.

Finally, the node_modules exclusion should be done in both, client and server.

@serut
Copy link
Owner Author

serut commented Sep 13, 2016

I released yesterday to have something to discuss, so sorry for regressions. Looks like /home/travis/build/serut/meteor-coverage/packages/local-test:lmieulet:meteor-coverage/server/tests.js is not rewritten correctly and made the error, so it's related to SourceMap algorithm.
I am not super exited to exclude the node_modules entierely, but if that's required, let's do that.

"test:packages-coverage": "spacejam test-packages --driver-package practicalmeteor:mocha-console-runner --coverage out_lcovonly"

[PCKGS]
"test": "spacejam test-packages ./ --coverage out_lcovonly --driver-package practicalmeteor:mocha-console-runner",
Copy link

@cgalvarez cgalvarez Sep 13, 2016

Choose a reason for hiding this comment

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

{Install spacejam as a globally available NPM package and execute spacejam test...} OR {install it as a local NPM dev-dependency and execute node_modules/.bin/spacejam test... or node_modules/spacejam/bin/spacejam test... or better, $(meteor npm bin)/spacejam test...}, but not mix both 😉

Otherwise, users will have to add the binaries path to their $PATH environment variable in some way, like export PATH="$PATH:./node_modules/.bin" or export PATH="$PATH:./node_modules/spacejam/bin" to get the package.json script commands working.

@cgalvarez
Copy link

cgalvarez commented Sep 13, 2016

Some findings:

  1. Executing meteor test-packages --driver-package practicalmeteor:mocha --settings settings.coverage.json tests my package and lmieulet:meteor-coverage, generates my package coverage reports (because of the glob patterns) and makes the HTML report available on localhost:3000/coverage.
  2. Executing meteor test-packages ./ --driver-package practicalmeteor:mocha --settings settings.coverage.json tests only my package, doesn't generate report on the output folder, and thus the HTML report on localhost:3000/coverage is empty, which is why I opened HTML report on live meteor package testing is empty #23.

Any idea about what could be happening here?

More on this: I've redirected the output of the server console to text files and made a diff which shows:

    1. includes all the packages (which is a bit weird, because I execute the command in my package folder; only lmieulet:meteor-coverage and my package are visible because are the only two packages that contain tests).
  • Definitely the BIG difference is that 2) stops after echoing MochaRunner.runServerTests: failures: 0, and 1) starts exporting coverage reports right after that same message, which explains why the output folder is empty, but not why this is happening.

@cgalvarez
Copy link

If you can start to work from this branch, it will be easier to integrate. For me, it's better if you create a pull request to integrate into serut/dev-1.0.0, I accept the pr and the commit pops here. PR #22 lacks some fix that serut/dev-1.0.0 has, but do as you can.

You're right. I noticed that before reading your answer, and did it that way. Let me know what you think about the algorithm. It applies a lot of changes, but better IMO.

It can be done by creating a new widget displayed by practicalmeteor:mocha on the test page that makes the HTTP calls, otherwise it's not possible.

I apologize if I explained bad. I was wondering how the coverage gets re-calc after rerunning the tests (the coverage-watch feature you mention in the readme.md). When I exec npm run test:packages-coverage-watch, I can see the tests results at localhost:3000 and the coverage HTML report at localhost:3000/coverage. If I make any changes to the package code, the tests are re-run, localhost:3000 gets updated, and looking at the server console logs, after mocha runner has finished its tests the coverage reports start generating. What I would like to know it's exactly that, how the reports start generating automatically when the tests are re-run.

@serut
Copy link
Owner Author

serut commented Sep 16, 2016

looking at the server console logs, after mocha runner has finished its tests the coverage reports start generating. What I would like to know it's exactly that, how the reports start generating automatically when the tests are re-run.

I don't follow you: there is no algorithm that creates reports on watch test ! meteor-coverage doesn't know that tests are done and can't produce reports by itself.

@cgalvarez
Copy link

Ok, I'll see what's happening here then. Thank you very much 😉

…s where not reliable + lint + still a bug with client coverage that is not correcty exported in reports
@cgalvarez
Copy link

Don't know when you plan to release v1.0.0, but I've found some more SourceMap issues that already fixed but not commited yet in a new PR, and still investigating the issues you comment on merged PR #33 because I'm facing them right now.

IMO would be desirable to have them all fixed before releasing v1.0.0 😊. Just letting you know about my intentions to fix these 🪲🪲🪲 and their existence. I'm currently working on them.

@serut
Copy link
Owner Author

serut commented Sep 17, 2016

I pushed meteor-coverage-app-exemple with the current branch and it failed a little bit, so I dont plan to release before both meteor-coverage and meteor-coverage-app-exemple are green. Anyway with your pull request all files are a lot better mapped to real files, so again thanks you very much for your awesome work. We can release the next week maybe ?

@cgalvarez
Copy link

We can release the next week maybe?

I'm giving my best effort to fix this issue definitely ASAP 😉 , but it's a fact that I can't fix what I don't understand yet 😇 .

so again thanks you very much for your awesome work

Thanks to you. You've made this awesome package. I'm only helping to improve it a bit.

I pushed meteor-coverage-app-exemple with the current branch and it failed a little bit

I tested serut/meteor-coverage-app-exemple for first time. The commands I've run are the same as in .travis.yml:

  • meteor npm run coverage-app-unit
  • meteor npm run coverage-app-full
  • meteor npm run coverage-packages-mocha

I saw some Altered source could not be accessed messages in the console. I suppose that's what you mean with it failed a bit. They are already fixed in the upcoming PR. If the issues are others, please, let me know.


Let's go with another issue: if I exec the following commands inside the meteor-coverage folder:

  1. spacejam-mocha test-packages ./ --coverage "out_lcovonly out_html": I get all the package files in the HTML report.
  2. meteor npm run test:packages-coverage-watch: the HTML report lacks some files.

This is very weird. The difference that surely makes those files missing in the reports is that (1) shows Module load hook: transform [/tmp/meteor-test-run1yxq98/.meteor/local/build/programs/server/packages/lmieulet_meteor-coverage.js in its verbose output, while (2) no. That file is the merged file of the entire package, and its source map contains the references to all the package files, which would be included if that file would be hooked by Istanbul.

Is this what you commented on PR #33? Any idea of what could be happening here?

@cgalvarez
Copy link

I've made PR #34 to fix these issues and another PR #4 to meteor-coverage-app-exemple to cover the two specific cases that I was testing with my own package because I forgot to test them in the previous PR. This way we ensure they will always be tested.

[PCKGS]
"test": "spacejam-mocha test-packages ./ --coverage out_lcovonly ",
"test:watch": "meteor npm run lint:fix & meteor npm run test:packages-coverage-watch",
"test:packages-coverage-watch": "meteor test-packages --driver-package practicalmeteor:mocha --settings settings.coverage.json",
Copy link

@cgalvarez cgalvarez Sep 18, 2016

Choose a reason for hiding this comment

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

Just discovered that there exist a shorten form for meteor test-packages --driver-package practicalmeteor:mocha ... too: meteor-mocha .... 🤘

spacejam-mocha test-packages --coverage out_lcovonly is wrong, it should be spacejam-mocha --coverage out_lcovonly.

According to the spacejam code:

  • spacejam-mocha = spacejam test-packages --driver-package=practicalmeteor:mocha-console-runner "$@"
  • meteor-mocha = meteor test-packages --driver-package=practicalmeteor:mocha "$@"

The two short forms can only be used when testing packages.

@cgalvarez
Copy link

cgalvarez commented Sep 19, 2016

Sorry, moved this comment to its own issue #35 .

…remap. Allow typescript users to remap their coverage.
@serut
Copy link
Owner Author

serut commented Sep 19, 2016

  • Fix remap timeout or replace it by xit to ignore the test
  • Edit readme.md with how to use remap
  • Remove legacy code inside reporters because now files are merged correctly mapped with real ones
  • Edit changelog
  • meteor-coverage-app-exemple build ok : Build Status 👋

@serut serut mentioned this pull request Sep 19, 2016
3 tasks
@cgalvarez
Copy link

cgalvarez commented Sep 19, 2016

@serut You merged the code of the branch new/remap in my fork. I didn't the PR because I was still testing it against meteor-coverage-app-exemple. The failed CI tests are due to it. I'll take a look at it.

EDIT: I'll start fixing that checklist tomorrow. I faced the timeout before, but it's due to an error, remap is fast.

@HugoHeneault
Copy link

Got some ETA for this?

@serut
Copy link
Owner Author

serut commented Sep 20, 2016

Very soon !

@coveralls
Copy link

Coverage Status

Coverage increased (+0.9%) to 75.969% when pulling 9bfcda0 on dev-1.0.0 into 7075ff7 on master.

* some es5 to es6, coverage improvement and fix script commands

* remove legacy code and fix readme info

* revert handlers export

* don't run ghost test

* set spacejam as devdep

* fix script commands with new spacejam syntax
@coveralls
Copy link

Coverage Status

Coverage increased (+3.3%) to 78.356% when pulling 02e02af on dev-1.0.0 into 7075ff7 on master.

@serut serut merged commit 32e30fb into master Sep 21, 2016
@serut serut deleted the dev-1.0.0 branch September 22, 2016 06:46
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

5 participants