GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Disable CI status change comments unless the "automerge" label is used by ambv · Pull Request #578 · python/miss-islington

Conversation

ambv commented Sep 9, 2022

Copy link

Copy Markdown

Contributor

Works around #577

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Disable CI status change comments unless the "automerge" label is used

b19c203

Works around #577

codecov Bot commented Sep 9, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Codecov Report

Merging #578 (fef2c2f) into main (f89f450) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #578   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines         1032      1017   -15     
  Branches        72        71    -1     
=========================================
- Hits          1032      1017   -15

Impacted Files	Coverage Δ
tests/test_status_change.py	`100.00% <ø> (ø)`
miss_islington/status_change.py	`100.00% <100.00%> (ø)`

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Merge failure cases

fef2c2f

ewdurbin temporarily deployed to miss-islingt-disable-st-hoqscr

September 9, 2022 15:09

Inactive

ambv merged commit 6326600 into main

Sep 9, 2022

ambv deleted the disable-status-check-comments branch

September 9, 2022 15:12

ezio-melotti reviewed

Sep 9, 2022

View reviewed changes

Comment thread

miss_islington/status_change.py

Comment on lines -109 to -114

		if failure:
		if failure or not success:
		emoji = "❌"
		status = "it's a failure or timed out"
		elif not success:
		emoji = "❌"
		status = "it's a failure"

ezio-melotti Sep 9, 2022

Copy link

Copy Markdown

Member

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.

Was this change intentional?

@DanielNoord recently fixed this.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

DanielNoord Sep 23, 2022

Copy link

Copy Markdown

Contributor

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.

@ambv Was this indeed intentional? If not I could open a PR to revert it to my last fix?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ezio-melotti reacted with thumbs up emoji

All reactions

👍 1 reaction

DanielNoord Sep 30, 2022

Copy link

Copy Markdown

Contributor

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.

@ezio-melotti I checked in cpython and it seems to be okay, although there are some weird comments that are probably due to checks timing out and then being rerun.

It doesn't regress on #576 but it does remove the narrowing down that the "it's a failure" comment gives. Previously if that was the only thing in the comment you knew there wasn't a timeout, so there was likely an actual issue with the PR. You no longer get this information, but I'm not sure it warrants a PR to re-introduce that. It doesn't seem to helpful anyway.

That said, it could be simplified to if not success. If failure == True then success will always be False, so that doesn't need double-checking. Want me to open a PR for that?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ezio-melotti Oct 4, 2022

Copy link

Copy Markdown

Member

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.

In python/cpython#97820 (comment) I got the Status check is done, and it's a failure or timed out ❌. message, even though there was no evident failure. python/cpython#97821 (comment) reported success.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

DanielNoord Oct 4, 2022

Copy link

Copy Markdown

Contributor

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.

It's hard to debug these comments after the fact.
https://api.github.com/repos/python/cpython/commits/66eca5d2f185cd6943afeb69dae3eba625bbd077/status
https://api.github.com/repos/python/cpython/commits/66eca5d2f185cd6943afeb69dae3eba625bbd077/check-runs

Both show that there are no failed runs. So I don't know what is going on there honestly... But I'm also fairly sure it isn't caused by the changes in this PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ezio-melotti reacted with thumbs up emoji

All reactions

👍 1 reaction

ambv Oct 4, 2022

Copy link

Copy Markdown

Contributor Author

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 cannot find it anymore but I had an example where it was a timeout but TIMED_OUT was not set so the bot would report a failure. And since the best the message could say anyway was that it's either a failure or a timeout, I decided to just shorten it.

IIRC this change also let me not touch more tests, which otherwise I would have to alter due to the leave_comment=False change.

What's more worrying is that @ezio-melotti's example are still the bot leaving comments even those leave_comment=False is set. What gives?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ambv Oct 4, 2022

Copy link

Copy Markdown

Contributor Author

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.

Turns out this was due to a missing change that I now also cover in #583

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ambv Oct 4, 2022

Copy link

Copy Markdown

Contributor Author

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.

Also, I remember what I meant by "this change also let me not touch more tests". The change in this PR caused test coverage to fall due to one of those branches to not be exercised anymore.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

This branch was previously deployed

1 inactive deployment

miss-islingt-disable-st-hoqscr — fef2c2fa Deployed Sep 9, 2022 by ewdurbin

Reviewers

ezio-melotti

ezio-melotti left review comments

+1 more reviewer

DanielNoord

DanielNoord left review comments

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

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.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

ambv commented Sep 9, 2022

Uh oh!

codecov Bot commented Sep 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch was previously deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Sep 9, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.