-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
PR Cop 2.0 #5815
PR Cop 2.0 #5815
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #5815 +/- ##
==========================================
- Coverage 55.90% 55.67% -0.23%
==========================================
Files 655 655
Lines 26316 26316
Branches 2543 2543
==========================================
- Hits 14712 14652 -60
- Misses 10892 10956 +64
+ Partials 712 708 -4
*This pull request uses carry forward flags. Click here to find out more. see 16 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
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.
Looks good!
Couple questions:
- With this change, every PR must have either a milestone or the "no milestone" label?
- What does the developer see if a PR fails merge check due to missing milestone or
type:
label? Is it clear enough?
.github/PULL_REQUEST_TEMPLATE.md
Outdated
* [ ] Have you followed the guidelines in our [Contributing document](https://github.com/nasa/openmct/blob/master/CONTRIBUTING.md)? | ||
* [ ] Have you associated this PR with a `type:` label? | ||
* [ ] Have you associated a milestone with this PR? | ||
* [ ] Testing instructions included in associated issue OR is this a dependency/testcase change? | ||
### Author Checklist |
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.
can we just get a space before this to be consistent with the other headers?
.github/PULL_REQUEST_TEMPLATE.md
Outdated
* [ ] Have you checked to ensure there aren't other open [Pull Requests](https://github.com/nasa/openmct/pulls) for the same update/change? | ||
* [ ] Is this change backwards compatible? For example, developers won't need to change how they are calling the API or how they've extended core plugins such as Tables or Plots. | ||
|
||
### Contribution Checklist |
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.
Should we note that both authors and reviewers should verify this section?
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.
Yeah I'm confused about who the intended audience is here. How does it differ from the author checklist?
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.
Looks good! Just a suggestion and a formatting fix.
.github/PULL_REQUEST_TEMPLATE.md
Outdated
* [ ] Have you checked to ensure there aren't other open [Pull Requests](https://github.com/nasa/openmct/pulls) for the same update/change? | ||
* [ ] Is this change backwards compatible? For example, developers won't need to change how they are calling the API or how they've extended core plugins such as Tables or Plots. | ||
|
||
### Contribution Checklist |
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.
Yeah I'm confused about who the intended audience is here. How does it differ from the author checklist?
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.
- How does one know how to set a milestone? Can the system not require a milestone, but add "No milestone" if one isn't entered?
- Similar question about type labels. Like the idea of automatically pulling from an issue, but what if the user hasn't tied an issue to the PR?
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 have some outstanding questions, didn't mark as "request changes" before, so marking that now so it's shown as reviewed
@shefalijoshi to review |
@unlikelyzero were you able to address my comments? I'm not seeing it |
Current Playwright Test Results Summary✅ 165 Passing - Run may still be in progress, this comment will be updated as current testing workflow or job completes... (Last updated on 12/14/2023 03:09:01pm UTC)
|
Test Case | Last 7 days Failures | Last 7 days Flakes |
---|---|---|
Telemetry Table unpauses and filters data when paused by button and user changes bounds
Retry 2 • Retry 1 • Initial Attempt |
0% (0)0 / 39 runsfailed over last 7 days |
17.95% (7)7 / 39 runsflaked over last 7 days |
📄 functional/plugins/notebook/restrictedNotebook.e2e.spec.js • 1 Flake
Test Case Results
Test Case | Last 7 days Failures | Last 7 days Flakes |
---|---|---|
Restricted Notebook with a page locked and with an embed @addinit Disallows embeds to be deleted if page locked @addinit
Retry 1 • Initial Attempt |
0% (0)0 / 39 runsfailed over last 7 days |
41.03% (16)16 / 39 runsflaked over last 7 days |
📄 functional/plugins/imagery/exampleImagery.e2e.spec.js • 1 Flake
Test Case Results
Test Case | Last 7 days Failures | Last 7 days Flakes |
---|---|---|
Example Imagery Object Can use Mouse Wheel to zoom in and out of latest image
Retry 1 • Initial Attempt |
0% (0)0 / 39 runsfailed over last 7 days |
15.38% (6)6 / 39 runsflaked over last 7 days |
📄 functional/plugins/notebook/notebookSnapshots.e2e.spec.js • 1 Flake
Test Case Results
Test Case | Last 7 days Failures | Last 7 days Flakes |
---|---|---|
Snapshot image tests Can drop an image onto a notebook and create a new entry
Retry 1 • Initial Attempt |
0% (0)0 / 38 runsfailed over last 7 days |
57.89% (22)22 / 38 runsflaked over last 7 days |
|
@@ -14,8 +14,10 @@ Closes <!--- Insert Issue Number(s) this PR addresses. Start by typing # will op | |||
|
|||
* [ ] Changes address original issue? | |||
* [ ] Tests included and/or updated with changes? | |||
* [ ] Command line build passes? |
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.
no longer necessary with our ci
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.
LGTM!
Contributes towards /issues/5755
Describe your changes:
All Submissions:
Author Checklist
Reviewer Checklist