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

Fix for silent plugin failures. #199

Merged
merged 1 commit into from
Jun 30, 2014

Conversation

aloysbaillet
Copy link
Contributor

This change adds a junitxml functional test that tries to create a xml report in a folder that doesn't exist.
The IO error was not being propagated to the user, so I added a log.exception (that appears in the logging stream in stdout), and a more compact stderr message.

An important side-effect of this change is that now any plugin failure will not stop nose from continuing. I believe this is good as it prevents a bad plugin from breaking the test process, but this could be sometimes bad, in the case of an early failure that should really prevent anything else from happening... that said it looks like I haven't broken anything at this stage.

I'm happy to revisit the error messages, or maybe use log.debug('...', exc_info=True) to only display the stacktrace when -v is passed to nose, looking for feedback on this.

This fixes #159.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.07%) when pulling e5d006f on aloysbaillet:fix_issue_159 into 5896c6b on nose-devs:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) when pulling d8f9ec4 on aloysbaillet:fix_issue_159 into 5896c6b on nose-devs:master.

@aloysbaillet
Copy link
Contributor Author

Code slightly cleaner and stacktrace only logged if -v is passed to nose2.

I'm now also considering adding some early flight checks in the junitxml plugin, to make the error slightly more explicit.

@thedrow
Copy link
Member

thedrow commented Jun 2, 2014

If an unexpected exception occurs I prefer nose2 to halt (but gracefully so print the error, log it and sys.exit(1)).
The plugin error handling should occur in the plugin because it's part of the plugin's logic.

I hope that make sense. If you feel differently, you can convince me. I'm not entirely sure what's right so I'm currently -0 on this PR.

@aloysbaillet
Copy link
Contributor Author

Thanks, I'm OK with terminating gracefully as well.
I know this code comes from the original unittest2 plugins branch, and there's no more indication there to what the "philosophy" of plugin failures should be.
The plugin can't do much in this case as it was given wrong inputs. It could even just print the error and decide not to fail... but I think I'll let the plugin throw the exception, as it is an exceptional case and from the plugin's point of view, this is a failure.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling 6b3d82d on aloysbaillet:fix_issue_159 into 5896c6b on nose-devs:master.

@aloysbaillet
Copy link
Contributor Author

So I updated nose2 to abort if an exception is caught.
Expected output for this test is now:
Internal Error: runTests aborted: [Errno 2] JUnitXML: Parent folder does not exist for file: '/does/not/exist.xml'
With -v you would get the full stack trace.

I also moved the error handling code into main.py instead of events.py, as calling sys.exit made more sense there. It also reverts events.py's Hook class to the original implementation from unitest2's plugins branch.

@aloysbaillet
Copy link
Contributor Author

Just a quick ping... is everything OK with the new patch?

@thedrow
Copy link
Member

thedrow commented Jun 29, 2014

Sorry, I was very busy.
This is a fix and a very good one.
But it doesn't fix #159. We need to find where nose2 opens the stream and report that the file does not exist there. The plugin should handle expected failures.

@aloysbaillet
Copy link
Contributor Author

No problem!
I thought this commit (which arrived a bit later sorry) would be doing just that: aloysbaillet@a852477
Do you prefer I squash all commits from this branch into a single one so it's easier to review? (I'm used to ReviewBoard which displays all commits in a single review... I can see GitHub's way is a bit confusing to follow multiple commits in a PR...)

@thedrow
Copy link
Member

thedrow commented Jun 30, 2014

@aloysbaillet Yes please. aloysbaillet@a852477 fixes #159.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling d39c39d on aloysbaillet:fix_issue_159 into 97c3231 on nose-devs:master.

thedrow added a commit that referenced this pull request Jun 30, 2014
@thedrow thedrow merged commit 811928a into nose-devs:master Jun 30, 2014
@aloysbaillet aloysbaillet deleted the fix_issue_159 branch June 30, 2014 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Silent failure if junit tries to write report to missing directory
3 participants