Add script to post Travid build errors on PRs #1138

Merged
merged 5 commits into from Nov 6, 2016

Projects

None yet

4 participants

@agnivade
Member

Fixes #1066

@agnivade agnivade Adding script to post comment on a PR on failure
9e2b04a
@mention-bot

@agnivade, thanks for your PR! By analyzing the history of the files in this pull request, we identified @igorshubovych to be a potential reviewer.

@agnivade agnivade added the tooling label Oct 31, 2016
@ostera
Member
ostera commented Oct 31, 2016

wouldn't this run the tests twice?

scripts/post_pr_comment.py
+ test_result = sys.stdin.read().strip()
+ comment = (
+ """
+The [build](https://travis-ci.org/tldr-pages/tldr/builds/{build_id}) for this PR has failed with the following message -
@waldyrious
waldyrious Oct 31, 2016 edited Member

Can this line be wrapped? Also, instead of " -" maybe end the sentence with ":".

@waldyrious
Member

Adding some documentation to the script would be nice to allow others to chime in if edits are needed in the future :)

One thing I failed to figure out, for example, is where the comment_body contents comes from. Care to elucidate?

@waldyrious waldyrious changed the title from Adding script to post comment on a PR on failure to Add script to post Travid build errors on PRs Oct 31, 2016
@agnivade
Member
agnivade commented Oct 31, 2016 edited

@waldyrious - Sure, will documentation document. You mean the comment_body that is part of the string formatting ?

comment_body is just replaced by the test_result variable which gets its content from stdin. Let me know if its not clear, you can comment on the code diff so I can get the line no. exactly.

@ostera - That's right. But I couldn't get any other way to get the contents of the test failure. Travis has no such env var. There was one called TRAVIS_FAILURE_RESULT but that just has the exit code. I am open to any better ideas.

EDIT: typo

@agnivade
Member

Not bad ! And we should not be worried about cleaning the file later right ? Because its in travis env anyway.

@ostera
Member
ostera commented Oct 31, 2016

Wxactly, the file will just go away

@agnivade agnivade Saving the test result to a file
- And reading from the file in case of failure
2f25be0
@waldyrious
Member

comment_body is just replaced by the test_result variable which gets its content from stdin.

Ok, that clarifies it, thanks. I had missed the test_result = sys.stdin.read().strip() line. Some comments (like the sentence I'm quoting above) would help following the data flow through the scripts.

@agnivade agnivade Adding comments to the script
c3bb67e
@agnivade
Member
agnivade commented Nov 1, 2016

@waldyrious - how does it look now ?

@waldyrious
Member

@agnivade better, thanks! Are you still going to change the message to be posted, per the previous comments?

@agnivade agnivade Changing punctuation
355e99c
@agnivade
Member
agnivade commented Nov 3, 2016 edited

@waldyrious - PTAL

scripts/post_pr_comment.py
+ # Populate the template text
+ comment = (
+ """
+The [build](https://travis-ci.org/tldr-pages/tldr/builds/{build_id}) for this PR has failed with the following message:
@waldyrious
waldyrious Nov 3, 2016 Member

Is there a reason the line wrapping couldn't be done?

@agnivade
agnivade Nov 3, 2016 Member

Sorry, what do you mean by "wrapping" here ?

@waldyrious
waldyrious Nov 4, 2016 Member

line breaking :) to make the source code more readable.

@agnivade
agnivade Nov 4, 2016 Member

But that will split the output into separate lines. :( I just tried it.

@waldyrious
waldyrious Nov 4, 2016 Member

Oh, ok. That's too bad, but no problem (that's why I asked whether there was something that made this undesirable).

@ostera
ostera Nov 5, 2016 edited Member

Here's an alternative way:

~ λ cat test.py

comment = (
"The [build]"
"(https://travis-ci.org/tldr-pages/tldr/builds/{build_id})"
" for this PR has failed with the following message:"
"\n```\n"
"{comment_body}"
"\n```\n"
).format(build_id=1234, comment_body="waat")

print comment

Which gives a result like this (the first backtick in the output is backslashed because Github's markdown renderer thought it was part of it's markdown, and not the body of the block):

~ λ py test.py
The [build](https://travis-ci.org/tldr-pages/tldr/builds/1234) for this PR has failed with the following message:
```
waat
```

And it looks like this:

The build for this PR has failed with the following message:

waat

[edit by @waldyrious: worked around the need to escape the backticks by using indentation-based code block formatting]

@waldyrious
Member

LGTM :D

scripts/post_pr_comment.py
+```
+{comment_body}
+```
+Please rectify the error and make another push.
@ostera
ostera Nov 5, 2016 Member

s/rectify/fix ? Plenty of our users won't have the broadest of vocabularies, we might as well make this more accessible.

@ostera
ostera Nov 5, 2016 Member

Also s/error/error(s) -- it could potentially be many of them!

@ostera
ostera Nov 5, 2016 Member

Lastly, and push again feels more natural than to make another push.

@waldyrious
waldyrious Nov 5, 2016 Member

I think the build fails after the first error, so even if there are multiple problems, Travis will only report them one by one (not 100% sure though)

@ostera
ostera Nov 5, 2016 Member

@waldyrious it would depend on how we run the tests, but the linter itself could fail with multiple linting errors.

scripts/post_pr_comment.py
+ print f.read()
+
+
+if __name__ == '__main__':
@ostera
ostera Nov 5, 2016 Member

Considering we are invoking this through the interpreter, it shouldn't be necessary --

import sys

print sys.stdin.read().strip()

And calling it as python test.py < test.py prints itself without the need of the wrapping block.

If we wanted to future-proof this, I'd put the block within the if-name-main into a function that can be exported, and then within the block just call the function.

@agnivade
agnivade Nov 6, 2016 Member

I get that since we are directly calling the file, we don't need the if-name-main check, but what is the significance of the print sys.stdin.read().strip() line ? Can you expand on that a bit ?

@ostera
ostera Nov 6, 2016 Member

That was just part of my example --- we also happen to use that to get the stdin in your PR as well

@agnivade agnivade Applying finishing touches to the script
- Removing the if-name-main check
- Splitting the main string across multiple lines
- Making minor changes to the comment text
9553e38
@ostera
ostera approved these changes Nov 6, 2016 View changes
@agnivade
Member
agnivade commented Nov 6, 2016

👍

@agnivade agnivade merged commit 2a9060a into master Nov 6, 2016

4 checks passed

approvals/lgtm this commit looks good
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@agnivade agnivade deleted the comment_pr branch Nov 6, 2016
@andrea-82 andrea-82 added a commit to andrea-82/tldr that referenced this pull request Nov 20, 2016
@agnivade @andrea-82 agnivade + andrea-82 Add script to post Travid build errors on PRs (#1138) 04a1633
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment