-
Notifications
You must be signed in to change notification settings - Fork 2k
Conversation
throw e; | ||
.on('end', function() { | ||
console.log('E2E Testing complete'); | ||
process.exit(); |
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.
Can we be explicit that 0 means good?
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.
ok, so make this line process.exit(0);
..should I add a comment above that says // exit with success.
?
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.
Yeah that would work
@@ -277,6 +285,8 @@ module.exports = function (grunt) { | |||
grunt.registerTask('test', ['env:test', 'lint', 'mkdir:upload', 'copy:localConfig', 'server', 'mochaTest', 'karma:unit']); | |||
grunt.registerTask('test:server', ['env:test', 'lint', 'server', 'mochaTest']); | |||
grunt.registerTask('test:client', ['env:test', 'lint', 'server', 'karma:unit']); |
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.
Client should test anything for client, including e2e. That shouldn't be a separate task IMO.
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 prefer e2e defined as a separate task, and add it to the test
task. this would make it more modular no?
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.
Right, but client should test e2e 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.
I don't think e2e should be included in the client task. There's a clear separation between these types of tests from the others. Demonstrated by the separation of the tests in their respective folders; /client, /server, and /e2e
E2E tests behave much differently then the others, so they probably should be isolated. Adding them to the main test
task is a good idea though, so that we can run all tests at once.
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 concur with @mleanos. For now, I will add e2e to the test
task and keep the test:e2e
separate from the test:client
task.
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.
@mleanos client should really have 2 subfolders, unit and e2e. Both tests test client-side code. If the folder structure really bothers you, feel free to submit a PR because anyway /client is just client-side unit tests and e2e is client-side e2e tests.
@jloveland I think you can set this config to use grunt-protractor-runner instead.. I believe this is why the CI is failing. this is how I tested it locally. And we wouldn't need to add protractor as a project dependency if we rely on grunt-protractor-runner to handle it. I think the same would go for gulp as well. Using gulp-protractor. |
@mleanos, good call. I will push an update, hopefully it will fix CI. |
@jloveland I'm experiencing issues with running The tests are failing about 50% of the time for me. To reproduce, just run this grunt task. After it's completed run it again, and repeat the process. The selenium server is running each time, but the test complains about "Element is not clickable.." I'm wondering if there's an issue with the webdriver process staying open, or something along those lines. Can you test this on your end? |
I tested with Gulp, and I get the same behavior. However, Gulp outputs "Error: listen EADDRINUSE" at the beginning of the test, and continues to open Chrome & then the test fails; just as Grunt behaves. So this is hinting at an issue with a process not terminating correctly. I'm going to investigate further. @jloveland I'll be on Gitter, if you want to work on this together or share results. |
@mleanos per our gitter conversation, I could not reproduce on a mac with As long as Travis builds we should be ok, but maybe someone else out there can reproduce and we can open another ticket. |
9087c2f
to
15f7898
Compare
@jloveland Agreed. It may not be an actual issue with the tests/Grunt. But rather some combination of environmental factors on my side. Nice :) Just saw the tests passed. |
To further elaborate on the behavior I'm seeing, so that others coming along later can have this info... When I try to run the E2E tests immediately back to back, the tests fail. Using Gulp I get EADDRINUSE, which may hint at what is going on with Grunt. Either way, it appears that the server (or node) process is taking a while to end. If I wait a few minutes to run the tests again, they succeed as expected. Also, when changing protractor to use Firefox, the tests work just fine; even when I run them immediately after one another. I think we should keep an eye on this. However, like I said before, it may just be completely on my end. I randomly run into the EADDRINUSE; mostly when testing with Chrome. All in all, this LGTM. Thanks @jloveland |
@jloveland Squash! Otherwise looks good. :) Thanks |
15f7898
to
fdf1ad5
Compare
@lirantal any issues, or can I merge? |
LGTM |
1 similar comment
LGTM |
Fix grunt and gulp e2e tests, Fixes #929
fixing #929