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

Islington reports status check being a success but also reports ❌ when checks fail #564

Closed
DanielNoord opened this issue Jul 11, 2022 · 2 comments · Fixed by #565
Closed

Comments

@DanielNoord
Copy link
Contributor

I noticed this in python/cpython#94734 and this comment: python/cpython#94734 (comment)

The checks clearly fail:
https://github.com/python/cpython/pull/94734/checks

Islington reports the checks as both being a success but uses the emoji that's being used to indicate that the checks failed.

Relevant code seems to be:

if title_match or is_automerge:
success = result["state"] == "success" and not any(
elem in [None, "failure", "timed_out"]
for elem in all_check_run_conclusions
)
if leave_comment:
if success:
emoji = "✅"
else:
emoji = "❌"
message = f"Status check is done, and it's a {result['state']} {emoji} ."

This seems rather straightforward so I'm not immediately sure what is broken here. Can there be some sort of race condition where result["state"] is "success" on L103 but has changed when we reach L112?

@DanielNoord
Copy link
Contributor Author

Oh no, I'm stupid. This is actually pretty straightforward. If result["state"] == "success" but there are timed out checks then the f-string will still use "success" even though the if success check fails.
That should be easily fixable. Working on a PR.

@jacobtylerwalls
Copy link
Contributor

Interesting. I fixed this in #501 but it was re-caused by a faulty merge in #481. Goes to show the value of tests.

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 a pull request may close this issue.

2 participants