-
Notifications
You must be signed in to change notification settings - Fork 63
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
GPII-2307: Add combined code coverage reports. #508
Conversation
…ves "browser disconnect" errors.
CI job failed. Please visit http://lists.gpii.net/pipermail/ci/ for more details. |
@avtar, or whomever set up the CI jobs, this PR makes key changes to the test commands used. Looking at the failing browser tests, it really seems like this is using outdated commands. As an example, The browser tests should be run using "npm run test:browser" or "npm run test:browserVagrant", but they're trying to use the outdated Testem config file: |
In a side discussion on chat, @amb26 and I reviewed the workaround that loaded the tests individually in testem, and agreed that I would create two "test loaders", one for qunit-composite tests, in infusion, and the other for gpii-testem. This PR now uses both. IMO it should be reviewed once gpii-testem and infusion are, so that we can pick up at least published dev versions. |
CI job failed. Please visit http://lists.gpii.net/pipermail/ci/ for more details. |
CI job failed. Please visit http://lists.gpii.net/pipermail/ci/ for more details. |
I can finally get these tests to run consistently, see this comment on the gpii-testem PR. I will update this pull to remove previous approaches and advise when it is ready for review. |
@avtar @gtirloni, etc. The CI jobs are still failing, even though the tests now pass consistently in Vagrant. This is simply because the commands have changed, the old commands can only fail when run against this branch. We should discuss how to cleanly update the CI job definition, and when to do that. I would argue that the CI config should be updated once all other concerns are addressed in this PR, i.e. when we would otherwise merge. We would in short order confirm the job working, merge, and inform everyone else to pull in the changes so that their builds continue working. |
…s required to avoid "browser disconnect" errors.
@amb26, I have cleaned up the various previous approaches, this PR now includes the following:
This should be ready for further review. However, I would suggest reviewing the comparable infusion PR first. |
CI job failed. Please visit http://lists.gpii.net/pipermail/ci/ for more details. |
… used by default...
CI job failed. Please visit http://lists.gpii.net/pipermail/ci/ for more details. |
@simonbates and @cindyli, as you both are frequent contributors to universal, I was hoping to get one of you to help review this pull that:
There is a similar infusion pull as well. The concerns and approaches are similar, at least to me it makes sense for the same person to review both if they have the bandwidth. Do please let me know if there's any chance either of you can pick one or both of these up. Happy to answer a few questions during our overlap to give context if that would help. cc: @amb26 @colinbdclark |
@@ -4,21 +4,27 @@ | |||
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" /> | |||
<title>GPII Canopy MatchMaker tests</title> | |||
|
|||
<link rel="stylesheet" media="screen" href="../../../../../../node_modules/infusion/tests/lib/qunit/css/qunit.css" /> | |||
<link rel="stylesheet" media="screen" href="/node_modules/infusion/tests/lib/qunit/css/qunit.css" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change to use absolute paths rather than continuing with relative paths something that must happen for using testem? This change would not allow developers to run an individual browser test within a local browser using the own web server.
To provide more detail of my use case, I always have MAMP running on my machine to serve as a local web server. When writing a browser unit test, I usually open that file in my local browser using a url like http://localhost/gpii/node_modules/universal/gpii/node_modules/canopyMatchMaker/test/web/html/CanopyMatchMakerUtilitiesTests.html
for debugging. This path change causes the load of the test file with this approach throws many 404 Not Found
error. Is this method not supported with the switch to use testem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can run testem without passing the "ci" argument and load the tests within a local browser. The command would look like testem --file tests/testem.js
(if you have testem installed globally) or node node_modules/testem/testem.js --file tests/testem.js
if you don't have it installed. This allows you to use whatever browser you choose with both the tests that only require a browser and those that require server-side fixtures.
However, as I know many people use MAMP and python to host the browser tests, I will look into making relative paths work for both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I relativised all paths and confirmed that the Vagrant browser tests still pass. I also confirmed that the following works:
- From the root of the universal repo, run
python -m SimpleHTTPServer 8000
. - Open
http://localhost:8000/gpii/node_modules/canopyMatchMaker/test/web/html/CanopyMatchMakerUtilitiesTests.html
in a browser. - Confirm all tests pass.
@cindyli, I assume that the tests will now work with your setup as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the alternative with relative paths. This doesn't work with my setup yet because the inclusion of /testem.js
and /coverage/client/coverageSender.js
at line 26, 27 is still using absolute paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, Cindy. The /testem.js file is served by Testem itself, and won't work when hosting via other means. The second file is served by the gpii-testem test component. Do the missing includes cause any actual test errors? If so, please comment on the failing test and the errors seen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cindyli, look at your screenshot. 603 of 603 assertions passed. You would not have that summary if the test run had not completed, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, all-tests.html
passes regardless those errors. The screenshot was to show there are numerous errors instead of 2 errors you mentioned in the earlier comment. Sorry if that screenshot confuses you.
For running individual tests, after checking the one I thought was failed - gpii/node_modules/canopyMatchMaker/test/web/html/CanopyMatchMakerUtilitiesTests.html
, I found it actually passed but not in a proper qUnit layout, which make it look like hanging. Can you fix that? Seems it's 'cuz the qUnit css file at line 7 is still using absolute path. Also tried running other test files individually. They all look fine.
BTW, this test file is repeated twice in all-tests.html
at line 14 & 32. It's probably an opportunity to fix it here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I was thinking of the larger number of errors in my earlier comment. I'll fix the path and remove the dupe a bit later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cindyli, I have fixed the duplicate test and the broken path. Both of those are great catches, thanks as always for your incredible eye for detail.
I also updated the README with a warning about the 404 errors when hosting the tests yourself. Please review, I hope that will be good enough for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The improvement looks and works great to me. Thanks, @the-t-in-rtf.
CI job failed. Please visit http://lists.gpii.net/pipermail/ci/ for more details. |
1 similar comment
CI job failed. Please visit http://lists.gpii.net/pipermail/ci/ for more details. |
CI job failed. Please visit http://lists.gpii.net/pipermail/ci/ for more details. |
…f canopy matchmaker tests.
…thon, MAMP, et cetera.
I started up an instance of the Vagrant Windows VM used with |
CI job failed. Please visit http://lists.gpii.net/pipermail/ci/ for more details. |
CI job failed. Please visit http://lists.gpii.net/pipermail/ci/ for more details. |
Hi, All. After trying a bunch of things, I eventually found the root of the problem. @gtirloni was correct that this was an Express error, in fact it was an error returned by the Express instance Testem uses to host content. The 403 is triggered under various circumstances, as shown here in the code. In this case, the problem was that the working directory check failed, i.e. Testem considered the value of As the original On a side note, once I got the tests working I discovered that the JournalIdParserTests fail in IE. I ran the tests in master, it has the same problem. I saved the failure output [to pastebin](https://pastebin.com/gzqLyPCq and will create a ticket for that follow-up work. As the failure is not related to this work, I have disabled running browser tests in IE for now in this package. I will leave a note on that ticket suggesting that they reenable it once the failing tests are fixed. |
The ticket for the failing JournalIdParser tests is here: GPII-2471 |
CI job failed. Please visit http://lists.gpii.net/pipermail/ci/ for more details. |
I repeated the commands in sequence and they all passed on Win10 for me! 💯 |
CI job passed. |
ok to test |
CI job passed. |
ok to test |
CI job passed. |
@the-t-in-rtf, I'm trying the latest changes on my Mac and hit this error at running
Can you see if you can reproduce it? Thanks. |
Some additional information of my environment where the error above occurs: Mac 10.11.6, node 6.7.0, npm 3.10.3 BTW, all tests pass at running |
@cindyli, |
Sorry, I mean it definitely has caused problems in other packages, not sure if the Vagrant setup here suffers from the same problem where node_modules is shared between the host/client environment. |
@the-t-in-rtf yes, I did run |
Merged at 2936d16 |
Thanks for figuring it out and linking to the discussion, and thanks again to everyone who worked on this. Happy to have this one merged! |
Previously, this repo has lacked code coverage reporting for either the node or browser code. This PR uses the new gpii-testem library now under review to add both. See GPII-2307 for details.
As I had to rewrite most of the test commands anyway, I took the time to migrate from using Grunt:shell to using npm scripts for all tests. I updated the README to reflect the syntax for running tests.
An example of the combined coverage report is available here:
http://the-t-in-rtf.github.io/universal-coverage-20170329/index.html
I have shared early results informally with the group, the latest version filters out bundled libraries and presents combined browser and node coverage in a single report. This report will automatically be generated when npm test is run on the local machine, or in vagrant.