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

GPII-3053: Fixed tests to work with latest sinon-chrome. #11

Merged
merged 16 commits into from Sep 16, 2018

Conversation

Projects
None yet
3 participants
@jobara
Copy link
Collaborator

commented Aug 6, 2018

@jobara jobara force-pushed the jobara:GPII-3053 branch from 747671c to deeeb55 Aug 6, 2018

@the-t-in-rtf

This comment has been minimized.

Copy link
Member

commented Aug 6, 2018

Hi, @jobara. I tried to run the tests both in a standalone browser and in testem, and in both cases there were failures to load a client-side dependency within a browser session. The most recent failure I looked at mentions the "dist" directory. I see that you have a Grunt uglify task to build that, but nothing in your test prep calls that. I figured out how to run that task, but it's best if npm test works out of the box, i.e. if whatever prep steps are required are built into the process of installing the package and/or running the tests.

@the-t-in-rtf

This comment has been minimized.

Copy link
Member

commented Aug 6, 2018

OK, I will need more time to investigate, but I notice that you try to use the package's "dist" content in fixtures like all-tests.html (not OK, but probably harmless) and domEnactorTests.html (very much not OK). Whatever the root cause here, you will break your reviewer's brain and make a misleading coverage map if you instrument and collect coverage data against a rollup like this. You need to break out the individual requires that point to dist within your tests.

I also suspect pathing problems. If you look at the source of any of the extension files, they are not instrumented, which to me suggests you're pointing to the copy from the filesystem and not the instrumented copy hosted by the coverage server. If you look at this example in universal, you can see the kind of path-munging machine I had to build to make it so that the paths worked both in python-hosted tests and in Testem.

@the-t-in-rtf

This comment has been minimized.

Copy link
Member

commented Aug 6, 2018

Just to clarify, dist is only problematic for tests. It's awesome and preferred for examples and demos.

@the-t-in-rtf

This comment has been minimized.

Copy link
Member

commented Aug 6, 2018

(Assuming you make it so that the dist content is always available, that is.)

@jobara

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 7, 2018

@the-t-in-rtf I switched to using infusion-all.js for the tests because nothing in there needs to be instrumented and doesn't require a build step. However, I'm still running into the same issue with the tests not running and immediately getting the error "Uncaught Error: Assertion failure - check console for more details: Unable to look up name gpii.chrome.portBinding.bindPortEvents as a global function". If I look in the inspector, all of the files I would expect are present. So it doesn't seem to be an issue with the pathing. And again, they all run okay outside of testem.

@the-t-in-rtf

This comment has been minimized.

Copy link
Member

commented Aug 9, 2018

So, @jobara, we're at an earlier stage in the process, just getting the browser tests working with coverage, so I didn't notice a key detail, which is that you have not configured things to create a combined report. This involves configuring both nyc and testem to avoid blowing away each other's coverage results, and then adding a posttest step to create the final report. If it's OK with the two of you, I plan to write up two tickets against gpii-testem that we can try out here shortly.

The first ticket would cover generalising the code I've used to make test suites that are interchangeable between testem and local (hosted) testing, and documenting that with examples. I would test this with this package and submit a pull either against your pull or against master if you decide to merge before that's available (it should be today or tomorrow).

The second ticket would cover documenting the common use case of combining browser and node coverage reports, which needs to be done in this package.

@the-t-in-rtf

This comment has been minimized.

Copy link
Member

commented Aug 9, 2018

Turns out I'd already written the "advanced" docs, I just didn't find them on my first quick skim through the docs.

@amb26

This comment has been minimized.

Copy link
Member

commented Aug 9, 2018

Great that they are there - perhaps the docs can be restructured to make them a bit easier to find - e.g. having an index which explains what is in each file, or renaming "advanced" to something more descriptive of its contents

@the-t-in-rtf

This comment has been minimized.

Copy link
Member

commented Aug 9, 2018

It's a fair point, I will look at that as well. I believe that was a "two hop" link when obviously it comes up enough to merit a top-level link from the README.

@jobara

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 10, 2018

@the-t-in-rtf I've been looking at your advanced docs this morning. I think they are a bit out of date with the respect to the location of the node modules to execute from the npm scripts. I referenced them directly from their node_modules locations instead of through .bin. Also the fluid-nyc-config package doesn't seem to exist ( https://github.com/GPII/gpii-testem/blob/9430e741bca240e13c0438e239f6478619ca75e7/docs/advanced.md#standardising-the-options-used-by-nyc ).

@jobara

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 10, 2018

@the-t-in-rtf i'm also getting an error that the grade gpii.testem.coverageDataOnly doesn't exist.
https://github.com/GPII/gpii-testem/blob/9430e741bca240e13c0438e239f6478619ca75e7/docs/advanced.md#sample-testemjs-file

@the-t-in-rtf

This comment has been minimized.

Copy link
Member

commented Aug 10, 2018

Thanks, @jobara, I'll update the docs as part of my work to make a testem/local hosting harness. The coverageDataOnly grade was deprecated in a recent release, here's the one you should be (and I think were?) using:

https://github.com/GPII/gpii-testem/blob/9430e741bca240e13c0438e239f6478619ca75e7/docs/testem-component.md#gpiitesteminstrument

If you scroll down from there you'll see much newer instructions about combining coverage reports. I will pare down to one good and up-to-date document, but hopefully that's closer to what you need.

@the-t-in-rtf

This comment has been minimized.

Copy link
Member

commented Aug 13, 2018

@jobara, I just created a pull with a static function that should help with the path issues:

GPII/gpii-testem#11

I cut a dev release (2.1.4-dev.20180813T134945Z.289e7fb) with the new function. For your convenience, here are the associated docs:

https://github.com/the-t-in-rtf/gpii-testem/blob/GPII-3278/docs/rollup.md

@the-t-in-rtf

This comment has been minimized.

Copy link
Member

commented Aug 14, 2018

I discovered a bug in gpii-testem that was preventing paths like %ui-options-chrome from being resolved in part of the testem lifecycle. I am fixing this at the moment. My plan is to submit a small pull against this branch with a known good testem.js configuration.

@the-t-in-rtf

This comment has been minimized.

Copy link
Member

commented Aug 14, 2018

I just submitted a small pull that updates to the newest dev release of gpii-testem and fixes the Testem setup and rollup fixture. See that pull for details.

jobara added some commits Aug 14, 2018

GPII-3053: Added default task
Necessary to prevent errors when trying to run tests through vagrant on 
a fresh box.
@jobara

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 14, 2018

@amb26 the tests are now running through npm test on the host machine and through npm run test:vagrant on a VM. This PR is ready for more review.

@@ -26,8 +26,14 @@
},
"homepage": "https://github.com/GPII/gpii-chrome-extension#README.md",
"scripts": {
"test": "node tests/node/all-tests.js",
"postinstall": "grunt build"
"pretest": "node node_modules/.bin/rimraf reports/* coverage/* && grunt build",

This comment has been minimized.

Copy link
@amb26

amb26 Aug 24, 2018

Member

This line doesn't function correctly in Windows

E:\Source\gits\gpii-chrome-extension\node_modules\.bin\rimraf:2
basedir=$(dirname "$(echo "$0" | sed -e 's,\\,/,g')")
          ^^^^^^^

SyntaxError: missing ) after argument list

This comment has been minimized.

Copy link
@jobara

jobara Aug 26, 2018

Author Collaborator

It seems that the problem on Windows is because rimraf and nyc were accessed through ".bin" instead of directly. See isaacs/rimraf#107 (comment)

@amb26

This comment has been minimized.

Copy link
Member

commented Aug 24, 2018

Please could you apply an update of all outdated dependencies, including gpii-grunt-lint-all and gpii.testem etc.

@jobara

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 26, 2018

@amb26 I made some changes to the test configuration. Things should be working now on Windows. This is ready for more review.

@amb26 amb26 merged commit 98e3bd4 into GPII:master Sep 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.