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

issues: use prefix match to check for existing issue #107444

Merged
merged 1 commit into from
Jul 24, 2023

Conversation

rafiss
Copy link
Collaborator

@rafiss rafiss commented Jul 24, 2023

It is conventional to annotate a test failure issue title with some additional information about the failure cause. The previous logic was too strict in requiring an exact title match. Now it will match as long as the title matches exactly OR if the prefix matches and is followed by whitespace.

Example:
I added a suffix to sql/tests: TestRandomSyntaxFunctions failed [panic in request_job_execution_details] since the issue was with a function the jobs team added. But the next day #107237 was opened anew. This PR would prevent that from happening.

Epic: None
Release note: None

It is conventional to annotate a test failure issue title with some
additional information about thef failure cause. The previous logic was
too strict in requiring an exact title match. Now it will match as long
as the title matches exactly OR if the prefix matches and is followed by
whitespace.

Release note: None
@rafiss rafiss requested a review from a team as a code owner July 24, 2023 13:57
@rafiss rafiss requested review from smg260 and renatolabs and removed request for a team July 24, 2023 13:57
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rafiss rafiss requested a review from tbg July 24, 2023 14:15
@renatolabs
Copy link
Contributor

Reusing an issue with a reason suffix (e.g. [panic in foo.go]) has the implicit assumption that every future failure of the test is caused by the same root cause; that's a dangerous assumption to make, especially when multiple comments on the same issue exist. So there would be an argument to be made that creating a new issue is not incorrect, since roachtest can't tell if the failure is the same or not.

Granted, this issue already exists in roachtest's default behaviour of commenting on existing issues when the title is unchanged. I somehow think this is worse in case there's a failure suffix in the issue title as it gives a notion of "issue has been triaged" and makes further inspection of individual failures in the thread less likely.

With all of that said, I agree there's not a lot to do in the short term here, and it's likely that in most cases, the assumption does hold that subsequent failures of the same test are due to the same root cause. In the absence of a better way to detect test failures and group them accordingly, this seems to be a pragmatic approach.

Thanks for your PR!

@rafiss
Copy link
Collaborator Author

rafiss commented Jul 24, 2023

Reusing an issue with a reason suffix (e.g. [panic in foo.go]) has the implicit assumption that every future failure of the test is caused by the same root cause; that's a dangerous assumption to make, especially when multiple comments on the same issue exist. So there would be an argument to be made that creating a new issue is not incorrect, since roachtest can't tell if the failure is the same or not.

Granted, this issue already exists in roachtest's default behaviour of commenting on existing issues when the title is unchanged. I somehow think this is worse in case there's a failure suffix in the issue title as it gives a notion of "issue has been triaged" and makes further inspection of individual failures in the thread less likely.

Very true point. The thread here also discussed this, and resulted in #87708 being created. If the test owner thinks the test can fail for multiple different reasons, then the X-noreuse label should be added.

@renatolabs
Copy link
Contributor

If the test owner thinks the test can fail for multiple different reasons

In my mind, every roachtest can fail for multiple different reasons. This PR is a tradeoff we're making that the convenience of grouping issues with a failure reason attached to the title compensates for the risk of masking different failure root causes by commenting on existing issues and those comments being overlooked.

I think convenience wins in this case due to existing limitations in roachtest.

@rafiss
Copy link
Collaborator Author

rafiss commented Jul 24, 2023

Thanks for the good discussion about tradeoffs and the nuances here.

bors r=renatolabs,tbg

@craig
Copy link
Contributor

craig bot commented Jul 24, 2023

Build succeeded:

@craig craig bot merged commit c917a6b into cockroachdb:master Jul 24, 2023
@rafiss
Copy link
Collaborator Author

rafiss commented Jul 25, 2023

blathers backport 23.1

@blathers-crl
Copy link

blathers-crl bot commented Jul 25, 2023

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 77f32c4 to blathers/backport-release-23.1-107444: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1 failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

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 this pull request may close these issues.

4 participants