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

Create template for failing tests. #21728

Merged
merged 25 commits into from
Nov 21, 2022

Conversation

ryanthompson591
Copy link
Contributor

@ryanthompson591 ryanthompson591 commented Jun 6, 2022

This is a template for when tests are red and makes creating failing test issues easier.


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Choose reviewer(s) and mention them in a comment (R: @username).
  • Add a link to the appropriate issue in your description, if applicable. This will automatically link the pull request to the issue.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests

See CI.md for more information about GitHub Actions CI.

@asf-ci
Copy link

asf-ci commented Jun 6, 2022

Can one of the admins verify this patch?

1 similar comment
@asf-ci
Copy link

asf-ci commented Jun 6, 2022

Can one of the admins verify this patch?

@github-actions github-actions bot added the build label Jun 6, 2022
Copy link
Contributor

@damccorm damccorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM, just had a few notes

Comment on lines 18 to 19
name: Bug Report
description: File a bug report
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These probably need to be changed to something like "Failing Test"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


If this isn't a bug and you have a question or support request, please email [email protected] with a description of the problem instead of opening this issue.
validations:
required: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be good to add a check box as well that asks something along the lines of "Is this a known flaky test?"

I expect someone reporting a flaky test would be reasonably likely to choose this template, so it would be good to cover that one. You could even add an entry to this file -

- valueFor: 'Priority'
- so that it automatically gets labeled "flaky" (docs on how that file works are here - https://github.com/damccorm/tag-ur-it)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like no checkboxes yet.
damccorm/tag-ur-it#21

How about a dropdown with an option to mark flakey or red.

@codecov
Copy link

codecov bot commented Jun 7, 2022

Codecov Report

Merging #21728 (5b84992) into master (c4cad83) will not change coverage.
The diff coverage is n/a.

❗ Current head 5b84992 differs from pull request most recent head f45389b. Consider uploading reports for the commit f45389b to get more accurate results

@@           Coverage Diff           @@
##           master   #21728   +/-   ##
=======================================
  Coverage   73.46%   73.46%           
=======================================
  Files         714      714           
  Lines       96497    96497           
=======================================
  Hits        70896    70896           
  Misses      24279    24279           
  Partials     1322     1322           
Flag Coverage Δ
go 51.45% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

- type: dropdown
id: flakey
attributes:
label: Issue Component
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be something like "Is this test flaky or consistently failing?"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this comment is resolved.

Comment on lines 43 to 44
- "flakey"
- "red"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- "flakey"
- "red"
- "Test is flaky"
- "Test is consistently failing"

Then in the tagging config we can search for the string "Test is flaky" and add the flaky label.

Also prefer spelling flaky instead of flakey for consistency with existing labels

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also update the wording to something like "Test is consistently failing"/"Test is flaky"? A newer contributor might not grok the difference between red and flaky (because a flaky test is sometimes red), and that gives us a string to search for in our labeler config.

Second, would you mind adding:

- contains: 'Test is flaky' # (or whatever string you decide on)
  addLabels: ['flaky']

to the rules in .github/issue-rules.yml

@aaltay
Copy link
Member

aaltay commented Jun 18, 2022

What is the next step on this PR?

Copy link
Contributor

@damccorm damccorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is waiting on the author. I also just added one more comment, once that and the other lingering comment are addressed we can merge

Copy link
Contributor Author

@ryanthompson591 ryanthompson591 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I fixed the merge problem. Have a look. One of the problems I have, is I don't have a good way to test this change to see if it works. Do you know how to do that?

- type: dropdown
id: flakey
attributes:
label: Issue Component
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this comment is resolved.

options:
- "Test is flaky"
- "Test is continually failing"
required: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
required: true
validations:
required: true

@damccorm
Copy link
Contributor

I think I fixed the merge problem. Have a look. One of the problems I have, is I don't have a good way to test this change to see if it works. Do you know how to do that?

If you merge it into master of your repo, you can try creating issues and validate that it does the right thing (that should give you the template).

If you don't want to do that in your main repo, you can copy the templates and labeler actions config to a test repo, that's a little more work but keeps your fork clean

@ryanthompson591
Copy link
Contributor Author

I tried this out in my repo and it worked fine. It's a good thing I tested this first because I needed a small change.

Copy link
Contributor

@damccorm damccorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@damccorm damccorm merged commit 4269e8a into apache:master Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants