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

Gracefully handle HTTP errors from pastebin #5764

Merged
merged 1 commit into from Aug 30, 2019

Conversation

@goerz
Copy link
Contributor

commented Aug 18, 2019

We find that the --pastebin option to pytest sometimes fails with "HTTP Error 400: Bad Request". We're still investigating the exact cause of these errors, but in the meantime, a failure to upload to the pastebin service should probably not crash pytest and cause a test failure in the continuous-integration.

This patch catches exceptions like HTTPError that may be thrown while trying to communicate with the pastebin service, and reports them as a "bad response", without crashing with a backtrace or failing the entire test suite.

@goerz goerz marked this pull request as ready for review Aug 18, 2019
@goerz

This comment has been minimized.

Copy link
Contributor Author

commented Aug 18, 2019

I don't understand what pytest-CI is complaining about.

Copy link
Member

left a comment

I'm surprised to hear of someone actually using this feature, I'd be interested to hear more about your usecase

don't worry about the test failure, #5752 will fix that 👍

response = (
urlopen(url, data=urlencode(params).encode("ascii")).read().decode("utf-8")
)
except OSError as exc_info: # urllib errors

This comment has been minimized.

Copy link
@asottile

asottile Aug 18, 2019

Member

I don't think we should silently gobble the errors here, if someone requests --pastebin and pytest fails to pastebin then the command should probably exit nonzero

This comment has been minimized.

Copy link
@goerz

goerz Aug 18, 2019

Author Contributor

Our use-case is a home-spun continuous integration script for https://www.qdyn-library.net. This project is hosted on a gitolite installation on a university workstation running commercial Fortran compilers, which also acts as the CI server via a simple post-commit hook kicking off a SLURM job to run the test matrix. Everything is totally headless and not web-based, so the CI-server sends developers an email with the results of the CI. These only include a summary, the detailed test output tends to be a bit too large to send around by email, so the CI-server uses --pastebin to upload the output, and puts a link in the email.

Recently, this started failing for some test runs with an HTTP 400 error. We're not quite sure what exactly the cause is. In any case, having the full test output available is very much secondary to the actual tests passing or failing. We don't want to get a "test failure" just because the upload to pastebin failed for some reason. After all, it might just be a temporary network failure. It would be enough to have the appropriate error message in the email (as this PR implements). We can then still investigate what the problem is, but a successful upload to pastebin is no longer a necessary condition for accepting patches. So to "silently gobble the errors" is explicitly the point here. The error will still be logged to the output, so it's not totally silent. Of course, this behavior is debatable. It's just that with the current behavior of pytest failing if pastebin fails, we'd probably have to stop using that feature and re-implement it as a pytest-plugin that matches our use case; that would obviously be more work than this small patch.

This comment has been minimized.

Copy link
@goerz

goerz Aug 18, 2019

Author Contributor

A possible compromise might be to exit with a specific status code that indicates "Everything OK except for the --pastebin". The CI-script might be able to catch that as a special case.

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Aug 19, 2019

Member

I'm OK with catching and displaying the error instead of returning non-zero. I think most (all?) use cases for --pastebin follow the pattern of "posting to pastebin for a nice-to-have report" but that is not really required.

Either way, this should target features as it changes the behavior. 👍

This comment has been minimized.

Copy link
@goerz

goerz Aug 20, 2019

Author Contributor

So should I rename changelog/5764.improvement.rst to changelog/5764.feature.rst and reword the changelog text?

This comment has been minimized.

Copy link
@goerz

goerz Aug 20, 2019

Author Contributor

I changed the base branch from master to features just now.

@blueyed

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

While at it: what about retrying it 3 times after e.g. 10, 30, 60 seconds?

@goerz

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

While at it: what about retrying it 3 times after e.g. 10, 30, 60 seconds?

That sounds like a good idea, but definitely a separate pull request

urlopen(url, data=urlencode(params).encode("ascii")).read().decode("utf-8")
)
except OSError as exc_info: # urllib errors
response = str(exc_info)

This comment has been minimized.

Copy link
@blueyed

blueyed Aug 20, 2019

Contributor

Would the exception info ever contain the URL?

Errors should get logged in any case probably. (not sure about non-zero exit code myself)

This comment has been minimized.

Copy link
@goerz

goerz Aug 20, 2019

Author Contributor

Probably not, but I can't give a guarantee. So it's probably best to revise this.

This comment has been minimized.

Copy link
@goerz

goerz Aug 20, 2019

Author Contributor

New commit 25dba82 addresses this

@goerz goerz changed the base branch from master to features Aug 20, 2019
@goerz

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

I'll see if I can change the test a little bit to increase patch coverage (if you care)

@blueyed

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

..appears to no have been rebased (but only the branch switched in the UI).
You need to rebase it locally and force-push (then you can also remove the mention :))

@goerz goerz force-pushed the goerz:pastebin branch from 25dba82 to 218e369 Aug 24, 2019
@goerz

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2019

I've force-pushed a new PR that is rebased directly on features. Sorry about the confusion. The revised code contains all of @blueyed's suggestions (no else statement for the try block).

This is still three commits (the patch itself, the changelog entry, and the tests), but I can squash it into a single commit if you prefer.

@codecov

This comment has been minimized.

Copy link

commented Aug 24, 2019

Codecov Report

Merging #5764 into features will decrease coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##           features    #5764      +/-   ##
============================================
- Coverage     96.33%   96.25%   -0.08%     
============================================
  Files           117      117              
  Lines         25995    26038      +43     
  Branches       2498     2499       +1     
============================================
+ Hits          25042    25063      +21     
- Misses          650      672      +22     
  Partials        303      303
Impacted Files Coverage Δ
testing/test_pastebin.py 100% <100%> (ø) ⬆️
src/_pytest/pastebin.py 91.66% <100%> (+0.43%) ⬆️
testing/code/test_code.py 95.76% <0%> (-4.24%) ⬇️
src/_pytest/doctest.py 95.04% <0%> (-1.99%) ⬇️
testing/python/fixtures.py 98.87% <0%> (-1.13%) ⬇️
src/_pytest/python_api.py 97.35% <0%> (-0.87%) ⬇️
src/_pytest/nodes.py 95.73% <0%> (-0.46%) ⬇️
testing/test_assertion.py 97.56% <0%> (-0.02%) ⬇️
src/_pytest/compat.py 98.54% <0%> (-0.02%) ⬇️
testing/test_session.py 100% <0%> (ø) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5bf9f9a...2931f6e. Read the comment docs.

Copy link
Member

left a comment

LGTM, thanks @goerz!

@nicoddemus

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

Linting is failing with:

flake8...................................................................Failed
hookid: flake8

src/_pytest/python_api.py:16:1: TYP002 @overload is broken in <3.5.2, add `if sys.version_info < (3, 5, 2): def overload(f): return f`
src/_pytest/recwarn.py:10:1: TYP002 @overload is broken in <3.5.2, add `if sys.version_info < (3, 5, 2): def overload(f): return f`

I suspect we just need to merge master into features: #5788.

@nicoddemus

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

@goerz could you please squash and rebase on the latest features one last time? Thanks! 🙇

@goerz goerz force-pushed the goerz:pastebin branch from 218e369 to 2931f6e Aug 27, 2019
We find that the --pastebin option to pytest sometimes fails with "HTTP
Error 400: Bad Request". We're still investigating the exact cause of
these errors, but in the meantime, a failure to upload to the pastebin
service should probably not crash pytest and cause a test failure in the
continuous-integration.

This patch catches exceptions like HTTPError that may be thrown while
trying to communicate with the pastebin service, and reports them as a
"bad response", without crashing with a backtrace or failing the entire
test suite.
@goerz goerz force-pushed the goerz:pastebin branch from 2931f6e to d47b9d0 Aug 27, 2019
@goerz

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2019

@nicoddemus

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

Awesome, thanks!

@nicoddemus

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

@blueyed @asottile would you like to do a 2nd round of reviews?

@asottile

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

I mean, the code looks fine -- retries would be nice, and I still think exiting 0 in this case is incorrect as pytest has failed to accomplish a thing that's explicitly asked for on the commandline

but I also would never use the pastebin feature so I don't really care about the outcome here 🤷‍♂

good enough for me :+0:

Copy link
Contributor

left a comment

Same for me.

@nicoddemus

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

Thanks guys!

@nicoddemus nicoddemus merged commit 404cf0c into pytest-dev:features Aug 30, 2019
6 checks passed
6 checks passed
WIP Ready for review
Details
codecov/changes No unexpected coverage changes found.
Details
codecov/patch 100% of diff hit (target 96.33%)
Details
codecov/project 96.33% (+<.01%) compared to 5bf9f9a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
pytest-CI #20190827.3 succeeded
Details
@goerz

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

Thanks!

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