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

[No QA] Add accessibility check to each PR in the deployChecklist #5117

Merged
merged 44 commits into from
Oct 19, 2021

Conversation

Julesssss
Copy link
Contributor

@Julesssss Julesssss commented Sep 7, 2021

CC: @roryabraham

Details

We are adding accessibility to newDot! Because of that we need to update our deploy checklists to accommodate for both QA and accessibility testing.

Accessibility failures will not prevent deploys as of now because nobody is writing accessible code. In the future we will be including accessibility checkboxes as a deploy blocker.

Old Format:

New Format:

Fixed Issues

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

Tests

SO MANY automated tests, and that's about the best we can do unfortunately. This can't be tested locally.

QA Steps

  1. Make sure the checklist is created in the new format defined above
  2. Make sure that when we add deploy blocker labels, it is added to the deploy checklist just like it currently does
  3. Make sure that when we CP to staging, they are added to the checklist

Tested On

n/a

Screenshots

n/a

@Julesssss Julesssss self-assigned this Sep 7, 2021
@stitesExpensify
Copy link
Contributor

Un-JK, the fixes were easier than I was expecting (thank god)

@stitesExpensify stitesExpensify marked this pull request as ready for review October 12, 2021 16:42
@stitesExpensify stitesExpensify requested a review from a team as a code owner October 12, 2021 16:42
@MelvinBot MelvinBot requested review from MonilBhavsar and removed request for a team October 12, 2021 16:43
Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

Thanks for including lots of unit test updates. It's really important to give us more confidence about the live-testing. Another thing that might be useful is to just print the result of the generateStagingDeployCashBody and paste it into a GitHub comment to get an idea what it looks like all rendered out. Only blocker is removing the accessibility stuff from the deploy blocker section.

.github/libs/GithubUtils.js Outdated Show resolved Hide resolved
.github/libs/GithubUtils.js Outdated Show resolved Hide resolved
tests/unit/GithubUtilsTest.js Outdated Show resolved Hide resolved
tests/unit/GithubUtilsTest.js Show resolved Hide resolved
.github/libs/GithubUtils.js Outdated Show resolved Hide resolved
@stitesExpensify
Copy link
Contributor

Here's the example checklist from createOrUpdateStagingDeploy #5782

@roryabraham
Copy link
Contributor

roryabraham commented Oct 12, 2021

Can we please add a - before each PR in the checklist? That way, GH will expand the PR title like this:

image

I think that will help reduce the cognitive load for Applause and deployers reviewing the checklist / make it easy to quickly check off any [No QA] PRs.

@stitesExpensify
Copy link
Contributor

WTF I literally just removed that, that was the "thing I have to fix" because it wouldn't work in my test on this PR's description. I will add it back. 6e47e40

@stitesExpensify
Copy link
Contributor

Updated, and updated the demo issue for the new format #5782

@Julesssss
Copy link
Contributor Author

Looks good, but I'm not going to officially review as I contributed with the early commits.

@stitesExpensify
Copy link
Contributor

Holding this until we're sure applause is ready to start

@stitesExpensify stitesExpensify changed the title Add accessibility check to each PR in the deployChecklist [HOLD] Add accessibility check to each PR in the deployChecklist Oct 13, 2021
@stitesExpensify stitesExpensify changed the title [HOLD] Add accessibility check to each PR in the deployChecklist Add accessibility check to each PR in the deployChecklist Oct 19, 2021
@stitesExpensify stitesExpensify merged commit d60834a into main Oct 19, 2021
@stitesExpensify stitesExpensify deleted the jules-deployChecklistAddAccessibleCheck branch October 19, 2021 20:58
@OSBotify
Copy link
Contributor

✋ 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
Copy link
Contributor

🚀 Deployed to staging by @stitesExpensify in version: 1.1.8-10 🚀

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

@roryabraham roryabraham changed the title Add accessibility check to each PR in the deployChecklist [No QA] Add accessibility check to each PR in the deployChecklist Oct 25, 2021
@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.10-2 🚀

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

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.

5 participants