Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

fixing grunt and gulp e2e tests #939

Merged
merged 1 commit into from
Oct 4, 2015
Merged

Conversation

jloveland
Copy link
Contributor

fixing #929
grunt-e2e-test

gulp-e2e-test

throw e;
.on('end', function() {
console.log('E2E Testing complete');
process.exit();
Copy link
Member

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?

Copy link
Contributor Author

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. ?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that would work

@ilanbiala ilanbiala added this to the 0.4.2 milestone Sep 27, 2015
@ilanbiala ilanbiala self-assigned this Sep 27, 2015
@@ -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']);
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

@mleanos
Copy link
Member

mleanos commented Sep 28, 2015

@jloveland I think you can set this config to use grunt-protractor-runner instead.. I believe this is why the CI is failing.
https://github.com/meanjs/mean/pull/939/files#diff-595bf3fa2348192244b0319be33066b8R214

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.

@jloveland
Copy link
Contributor Author

@mleanos, good call. I will push an update, hopefully it will fix CI.

@mleanos
Copy link
Member

mleanos commented Sep 28, 2015

@jloveland I'm experiencing issues with running grunt test:e2e

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?

@mleanos
Copy link
Member

mleanos commented Sep 28, 2015

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.

@jloveland
Copy link
Contributor Author

@mleanos per our gitter conversation, I could not reproduce on a mac with node 0.12.7 and chrome 45.0.2454.101 (64-bit).

As long as Travis builds we should be ok, but maybe someone else out there can reproduce and we can open another ticket.

@mleanos
Copy link
Member

mleanos commented Sep 29, 2015

@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.

@mleanos
Copy link
Member

mleanos commented Sep 29, 2015

To further elaborate on the behavior I'm seeing, so that others coming along later can have this info...
node 0.12.7
chrome 45.0.2454.101
Windows 7

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

@codydaig
Copy link
Member

@jloveland Squash! Otherwise looks good. :) Thanks

@ilanbiala
Copy link
Member

@lirantal any issues, or can I merge?

@lirantal
Copy link
Member

lirantal commented Oct 4, 2015

LGTM

1 similar comment
@codydaig
Copy link
Member

codydaig commented Oct 4, 2015

LGTM

ilanbiala added a commit that referenced this pull request Oct 4, 2015
Fix grunt and gulp e2e tests, Fixes #929
@ilanbiala ilanbiala merged commit 7a9ee53 into meanjs:master Oct 4, 2015
@jloveland jloveland deleted the e2e-tests-fix branch October 5, 2015 00:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants