-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
New behavior of the ``--pastebin`` option: failures to connect to the pastebin server are reported, without failing the pytest run |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 we should silently gobble the errors here, if someone requests
--pastebin
and pytest fails to pastebin then the command should probably exit nonzeroThere 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.
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.
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.
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.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'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. 👍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.
So should I rename
changelog/5764.improvement.rst
tochangelog/5764.feature.rst
and reword the changelog text?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 changed the base branch from
master
tofeatures
just now.