Add test to check for Contributor and Contributor+ Checklists #9772

AndrewGable · 2022-07-07T22:25:38Z

Details

Adds a check for a complete contributor and contributor plus checklist on every PR. It works as follows:

Triggered when a PR is created, body of the PR is edited, or a comment is made or edited.
Looks through the entire comment history
Looks and sees if it contains both the completed C and C+ checklist

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/214975

Tests

This test should fail on this PR at the start
I will check them all off and you should notice the error message will change
The "final reviewer" will leave a completed C+ checklist
The test should then pass

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Verify that no errors appear in the JS console

AndrewGable · 2022-07-07T22:33:32Z

Test is failing for a different reason than expected, I will look into it.

AndrewGable · 2022-07-08T17:10:22Z

Ok this is now failing as expected per the tests

AndrewGable · 2022-07-19T17:13:32Z

Tests still failing until a C+ (AKA reviewer) checks their checklist completely off.

amyevans

A couple thoughts, though not necessarily blockers

amyevans · 2022-07-19T18:45:55Z

.github/actions/javascript/contributorChecklist/contributorChecklist.js

+const _ = require('underscore');
+const GitHubUtils = require('../../../libs/GithubUtils');
+
+const issue = github.context.payload.issue ? github.context.payload.issue.number : github.context.payload.pull_request.number;


Are context, payload, and pull_request guaranteed to exist? Would be slightly safer to use optional chaining:

Suggested change

const issue = github.context.payload.issue ? github.context.payload.issue.number : github.context.payload.pull_request.number;

const issue = github.context?.payload?.issue ? github.context.payload.issue.number : github.context?.payload?.pull_request?.number;

amyevans · 2022-07-19T18:53:11Z

.github/actions/javascript/contributorChecklist/contributorChecklist.js

+        return;
+    }
+
+    console.log('All checklist are complete 🎉');


Suggested change

console.log('All checklist are complete 🎉');

console.log('All checklists are complete 🎉');

.github/workflows/testChecklists.yml

amyevans · 2022-07-19T19:33:36Z

.github/actions/javascript/contributorChecklist/contributorChecklist.js

+const contributorChecklist = `#### Contributor (PR Author) Checklist
+    - [x] I linked the correct issue in the \`### Fixed Issues\` section above
+    - [x] I wrote clear testing steps that cover the changes made in this PR
+        - [x] I added steps for local testing in the \`Tests\` section
+        - [x] I added steps for Staging and/or Production testing in the \`QA steps\` section
+        - [x] I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
+        - [x] I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
+    - [x] I included screenshots or videos for tests on [all platforms](https://github.com/Expensify/App/blob/main/CONTRIBUTING.md#make-sure-you-can-test-on-all-platforms)
+    - [x] I ran the tests on **all platforms** & verified they passed on:
+        - [x] iOS / native
+        - [x] Android / native
+        - [x] iOS / Safari
+        - [x] Android / Chrome
+        - [x] MacOS / Chrome
+        - [x] MacOS / Desktop
+    - [x] I verified there are no console errors (if there’s a console error not related to the PR, report it or open an issue for it to be fixed)
+    - [x] I followed proper code patterns (see [Reviewing the code](https://github.com/Expensify/App/blob/main/PR_REVIEW_GUIDELINES.md#reviewing-the-code))
+        - [x] I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. \`toggleReport\` and not \`onIconClick\`)
+        - [x] I verified that comments were added to code that is not self explanatory
+        - [x] I verified that any new or modified comments were clear, correct English, and explained “why” the code was doing something instead of only explaining “what” the code was doing.
+        - [x] I verified any copy / text shown in the product was added in all \`src/languages/*\` files
+        - [x] I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
+        - [x] I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named “index.js”. All platform-specific files are named for the platform the code supports as outlined in the README.
+        - [x] I verified the JSDocs style guidelines (in [\`STYLE.md\`](https://github.com/Expensify/App/blob/main/STYLE.md#jsdocs)) were followed
+    - [x] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
+    - [x] I followed the guidelines as stated in the [Review Guidelines](https://github.com/Expensify/App/blob/main/PR_REVIEW_GUIDELINES.md)
+    - [x] I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like \`Avatar\`, I verified the components using \`Avatar\` are working as expected)
+    - [x] I verified all code is DRY (the PR does not include any logic written more than once, with the exception of tests)
+    - [x] I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
+    - [x] If a new component is created I verified that:
+        - [x] A similar component does not exist in the codebase
+        - [x] All props are defined accurately and each prop has a \`/** comment above it */\`
+        - [x] Any functional components have the \`displayName\` property
+        - [x] The file is named correctly
+        - [x] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
+        - [x] The only data being stored in the state is data necessary for rendering and nothing else
+        - [x] For Class Components, any internal methods passed to components event handlers are bound to \`this\` properly so there are no scoping issues (i.e. for \`onClick={this.submit}\` the method \`this.submit\` should be bound to \`this\` in the constructor)
+        - [x] Any internal methods bound to \`this\` are necessary to be bound (i.e. avoid \`this.submit = this.submit.bind(this);\` if \`this.submit\` is never passed to a component event handler like \`onClick\`)
+        - [x] All JSX used for rendering exists in the render method
+        - [x] The component has the minimum amount of code necessary for its purpose and it is
+    - [x] If a new CSS style is added I verified that:
+        - [x] A similar style doesn’t already exist
+        - [x] The style can’t be created with an existing [StyleUtils](https://github.com/Expensify/App/blob/main/src/styles/StyleUtils.js) function (i.e. \`StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG\`)
+    - [x] If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like \`Avatar\` is modified, I verified that \`Avatar\` is working as expected in all cases)
+    - [x] If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.`;


Hmm, I feel like this approach of essentially duplicating the checklist text here and searching for its presence via includes has the potential to create headaches. If the checklist text changes between the opening of the PR and the time it's being reviewed, the check could fail until the author/reviewer hunts down the latest version of the list and copy/pastes it in. For example there have been already updates to the checklist such as this since the opening of this PR. (I suppose you could argue that's a feature not a bug, but 😐). Without digging into the workflow code a bit, it might not be obvious how to fix the failure.

Additionally, the duplication means the lists are prone to diverging (e.g. if someone updates PULL_REQUEST_TEMPLATE.md without updating this file). Would it be possible to read the contents of the MD file (or the PR body?), use a regex to isolate the checklist, then search for any unchecked boxes instead?

Without digging into the workflow code a bit, it might not be obvious how to fix the failure.

This is all true and fair feedback! I can update the error message, but will also update the tests with this new change from main.

Would it be possible to read the contents of the MD file

Yes, it should be possible to read the PR checklist template as it's in the repo, but I do wonder if it's the direction we want to head in? We've been discussing how to improve this and have been discussing possibly having a bot comment with the checklist depending on the contents of the PR.

Additionally, I did initially try doing this via regex, and it's a very nasty regex.

having a bot comment with the checklist depending on the contents of the PR

Oh nice, that does sound like a fabulous alternative

it's a very nasty regex

😬 Fair

Ok so for now can we use this as a "V1" and then improve with a 🤖 in the future for specific PR checklists?

Yep, I'm on board

AndrewGable · 2022-08-03T16:17:06Z

Bump

amyevans · 2022-08-03T20:29:08Z

As I check off each box it's triggering the GitHub action to run - I wonder if there's a way to delay that?

amyevans · 2022-08-03T20:39:17Z

I hadn't previously realized that checking off boxes that are in a <details> section closes the accordion each time, which is super obnoxious behavior GitHub 🙄 . Anyway, instead of continuing to use the UI to check them off, I edited the markdown in the OP, and that also prevents the GitHub Action from triggering a bunch of extra times.

However, now that all boxes are checked, the action is still failing @AndrewGable

AndrewGable · 2022-08-09T21:32:47Z

Thank you Amy, I will look into this soon.

AndrewGable · 2022-08-31T15:53:28Z

Ok @roryabraham and @amyevans this has been updated! I've tested it quite a lot and improved it from the V1. It now only runs on PR comments, PR reviews, and PRs that contain a checkbox item, so it should limit the noise.

.github/actions/javascript/contributorChecklist/contributorChecklist.js

AndrewGable · 2022-09-02T10:38:45Z

@roryabraham Do you want to review?

roryabraham · 2022-09-02T10:45:35Z

.github/actions/javascript/contributorChecklist/action.yml

+    description: Auth token for New Expensify Github
+    required: true
+runs:
+  using: 'node12'


Use node 16?

Ah yes - I will update to use nvm version

Guess we can't, I will use node16 👍

melvin-bot · 2022-09-02T10:55:03Z

@AndrewGable looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

AndrewGable · 2022-09-02T10:57:02Z

Tests had passed 👎

OSBotify · 2022-09-02T10:59:07Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2022-09-05T09:13:44Z

🚀 Deployed to staging by @AndrewGable in version: 1.1.97-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

AndrewGable added 2 commits July 7, 2022 16:17

Add test to check for Contributor and Contributor+ Checklists

eac0196

Update built JS file

214a4c4

AndrewGable requested a review from roryabraham July 7, 2022 22:25

AndrewGable requested a review from a team as a code owner July 7, 2022 22:25

AndrewGable self-assigned this Jul 7, 2022

melvin-bot bot requested review from amyevans and removed request for a team July 7, 2022 22:26

AndrewGable changed the title ~~Add test to check for Contributor and Contributor+ Checklists~~ [HOLD]Add test to check for Contributor and Contributor+ Checklists Jul 7, 2022

AndrewGable added 2 commits July 7, 2022 16:29

Move to correct repo

1f7d560

Also run on synchronize

ac5a0e7

AndrewGable added 5 commits July 8, 2022 10:40

Use current branch instead of main for checklist code

6e83bf6

Fix lint errors

7ea70fe

Update source

cab1e3a

Use branch name instead of no branch

a88386c

Use correct object

c68d7a8

AndrewGable changed the title ~~[HOLD]Add test to check for Contributor and Contributor+ Checklists~~ Add test to check for Contributor and Contributor+ Checklists Jul 19, 2022

amyevans reviewed Jul 19, 2022

View reviewed changes

AndrewGable added 6 commits August 29, 2022 12:59

Merge branch 'main' into andrew-checklist

7e3f6e4

Update checklists used in tests

cad96f2

Run on all issue_comment types

f07e80a

Log comments

bf0d04e

Tweak logs

87126f2

Tweak logs more

6114a30

AndrewGable added 6 commits August 31, 2022 17:20

Add better logging for unchecked items

624c976

Re-add core.setFailed

0fc1eb8

Improve logging

be0208e

Try core.error

7e1e6b6

Move back to setFailed

2ec357b

Rebuild actions

9e721e7

This comment was marked as outdated.

Sign in to view

AndrewGable added 2 commits August 31, 2022 17:44

Fix lint issues

8e36be9

Remove changes to package-lock.json

3fb05a4

AndrewGable requested a review from amyevans August 31, 2022 15:53

amyevans previously approved these changes Sep 2, 2022

View reviewed changes

Update from peer review comments

092432f

AndrewGable dismissed amyevans’s stale review via 092432f September 2, 2022 10:12

AndrewGable requested a review from amyevans September 2, 2022 10:21

amyevans previously approved these changes Sep 2, 2022

View reviewed changes

roryabraham reviewed Sep 2, 2022

View reviewed changes

Use node 16

5afc6b8

AndrewGable dismissed amyevans’s stale review via 5afc6b8 September 2, 2022 10:50

roryabraham approved these changes Sep 2, 2022

View reviewed changes

AndrewGable merged commit 74f5528 into main Sep 2, 2022

AndrewGable deleted the andrew-checklist branch September 2, 2022 10:54

melvin-bot bot added the Emergency label Sep 2, 2022

roryabraham removed the Emergency label Sep 2, 2022

OSBotify mentioned this pull request Sep 5, 2022

Deploy Checklist: New Expensify 2022-09-05 #10808

Closed

33 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test to check for Contributor and Contributor+ Checklists #9772

Add test to check for Contributor and Contributor+ Checklists #9772

AndrewGable commented Jul 7, 2022 •

edited

Loading

AndrewGable commented Jul 7, 2022

AndrewGable commented Jul 8, 2022

AndrewGable commented Jul 19, 2022

amyevans left a comment

amyevans Jul 19, 2022

amyevans Jul 19, 2022

amyevans Jul 19, 2022

AndrewGable Jul 20, 2022

amyevans Jul 20, 2022

AndrewGable Jul 21, 2022

amyevans Jul 21, 2022

AndrewGable commented Aug 3, 2022

amyevans commented Aug 3, 2022

amyevans commented Aug 3, 2022

AndrewGable commented Aug 9, 2022

This comment was marked as outdated.

AndrewGable commented Aug 31, 2022

AndrewGable commented Sep 2, 2022

roryabraham Sep 2, 2022

AndrewGable Sep 2, 2022

AndrewGable Sep 2, 2022

melvin-bot bot commented Sep 2, 2022

AndrewGable commented Sep 2, 2022

OSBotify commented Sep 2, 2022

OSBotify commented Sep 5, 2022

	const issue = github.context.payload.issue ? github.context.payload.issue.number : github.context.payload.pull_request.number;
	const issue = github.context?.payload?.issue ? github.context.payload.issue.number : github.context?.payload?.pull_request?.number;

	console.log('All checklist are complete 🎉');
	console.log('All checklists are complete 🎉');

Add test to check for Contributor and Contributor+ Checklists #9772

Add test to check for Contributor and Contributor+ Checklists #9772

Conversation

AndrewGable commented Jul 7, 2022 • edited Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

AndrewGable commented Jul 7, 2022

AndrewGable commented Jul 8, 2022

AndrewGable commented Jul 19, 2022

amyevans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewGable commented Aug 3, 2022

amyevans commented Aug 3, 2022

amyevans commented Aug 3, 2022

AndrewGable commented Aug 9, 2022

This comment was marked as outdated.

AndrewGable commented Aug 31, 2022

AndrewGable commented Sep 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melvin-bot bot commented Sep 2, 2022

AndrewGable commented Sep 2, 2022

OSBotify commented Sep 2, 2022

OSBotify commented Sep 5, 2022

AndrewGable commented Jul 7, 2022 •

edited

Loading