CI is reporting success when tests fail - Githubissues

mozilla / tofino

Project Tofino is a browser interaction experiment.

https://mozilla.github.io/tofino

Apache License 2.0

649 stars 67 forks source link

CI is reporting success when tests fail #647

Closed Mossop closed 8 years ago

Mossop commented 8 years ago

See the results at https://travis-ci.org/mozilla/tofino/jobs/135662381 for example. Even though a test timed out travis says everything worked. Appveyor is doing the same so this isn't a travis issue.

victorporof commented 8 years ago

I wonder if https://github.com/mozilla/tofino/pull/649 helps at all?

victorporof commented 8 years ago

@Mossop Found another problem: https://github.com/mozilla/tofino/pull/636#discussion_r66229583 Removing packages from package.json doesn't make the tests fail, even though packages continue to be used.

Mossop commented 8 years ago

@victorporof I'm pretty sure the problem is that npm test isn't returning a non-zero exit code in case of error, at least it seems that way when testing locally. You worked on the code that runs tests most recently, could you take a look here?

victorporof commented 8 years ago

@Mossop I'll have a look, but I'm not sure if any change I made could have made mocha failures not actually exit with a failure.

Regarding https://github.com/mozilla/tofino/issues/647#issuecomment-224548472 I believe it's because the node_modules folder isn't clobbered? A package was removed from package.json, but was still used somewhere else, and no test failed.

victorporof commented 8 years ago

Here's another example: https://github.com/mozilla/tofino/pull/652#event-686758380 CI reports everything as passing, but we were getting 403s https://ci.appveyor.com/project/Mozilla/tofino-u1hv8/build/1.0.538-svnbmaus/job/n4ycvuh03qehdbnf

victorporof commented 8 years ago

So https://github.com/mozilla/tofino/pull/660 should fix the originally reported problem (https://github.com/mozilla/tofino/issues/647#issue-159068329), we still have the issue of

node_modules folder isn't clobbered, resulting in no failures when modules are removed from package.json, but still being used in app code or tests. e.g. this should not succeed: https://ci.appveyor.com/project/Mozilla/tofino-u1hv8/build/1.0.535-qxxslnqf (the PR was since updated to reinclude the missing dependency)
npm install failures (e.g. by getting 403s) aren't reported as CI failures. e.g. this should not succeed: https://ci.appveyor.com/project/Mozilla/tofino-u1hv8/build/1.0.540-vctbwoao

Mossop commented 8 years ago

The node_modules thing is intentional caching to save time. We can npm prune at the start of things though. On Jun 9, 2016 08:36, "Victor Porof" notifications@github.com wrote:

So #660 https://github.com/mozilla/tofino/pull/660 should fix the originally reported problem (#647 (comment) https://github.com/mozilla/tofino/issues/647#issue-159068329), we still have the issue of

node_modules folder isn't clobbered, resulting in no failures when modules are removed from package.json, but still being used in app code or tests.

npm install failures (e.g. by getting 403s) aren't reported as CI failures.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mozilla/tofino/issues/647#issuecomment-224821425, or mute the thread https://github.com/notifications/unsubscribe/AAasTiYTpni1D511QsMx9xDI1sDMpLwuks5qJ8KagaJpZM4IwjHQ .

victorporof commented 8 years ago

403s are now reported and fail the build: https://travis-ci.org/mozilla/tofino/jobs/136648638 npm pruneing would be the last thing to fix.

victorporof commented 8 years ago

@Mossop: so how do you feel about pruning? Expecting to take that soon?

Mossop commented 8 years ago

Sorry, was preoccupied with other stuff. Happy to take pruning, I can take it tomorrow if you don't want to write it.

Mossop commented 8 years ago

Hopefully this is fixed by #695.