Milo PRs that are Ready for Review & Merging

[hparra]

Guidelines

Milo PRs that are Ready for Review:

• have all checks passing ✅
• are not drafts
• are not approved or awaiting changes requested

Why does this matter?

Reviewing code is a big deal. We're basically saying: "I looked at your code and checked test urls and found no issues so I'm signing off on this". If you take this perspective, it's easy to see why devs do not review PRs that are failing checks, missing test urls, missing docs, or are just too big (loc).

Speaking for myself, I only review PRs with green checks. Many times I won't even click into a PR with a red check, and I definitely will not approve a PR with failing UTs or a decrease in coverage.

Let's give each other all the assurances we need to approve PRs.

How do I get all checks green?

1. Adobe CLA Signed?
   • Ensure you are part of adobe and adobecom orgs
2. Check code compatibility with Milo supported browsers / check
   • Ensure you are not using unsupported DOM APIs, etc.
3. CodeQL / Analyze (javascript)
   • Ensure there are no known security vulnerabilities in your code
4. Code scanning results / CodeQL
   • Ensure there are no known security vulnerabilities in your code
5. codecov/patch
   • Ensure your PR has 100% test coverage
   • Sometimes we just have to pay off someone else's debt
6. codecov/project
   • Ensure your PR has 100% test coverage
   • Should pass if patch passes
7. Enforce PR labels / enforce-label
   • Ensure you have added the needs-verification label or zero-impact
   • Verifier will switch needs-verification to "verified", if applicable
   • Add the "Ready for Stage" label once the PR is ready to be merged
8. helix-psi-check
   • Always add at least one URL specific to the repo, even if it's a dummy
   • Helix bot won't check non-repo-specfic URLs
   • Ensure score is above 90
   • If the tests fails due to flakeyness, you can try to re-run or use hlx.live pages which are more stable.
9. Nala Tests / Running E2E & IT
   • This is usually skipped!
10. Unit Tests / Running tests (18.x)
    • Ensure your tests pass!
    • Sometimes a flakey test fails -- if so rerun the tests
11. Lint / Running eslint
    • NEW: Ensure your code is linted
    • See #988

The most common reasons I see ❌ are due to UTs (10), labels (7) and PSI (8).

Several changes have been made over the last few months to improve flakey tests and CI details. It's no longer unreasonable to expect PRs to have all checks passing. There will always be some exceptions, e.g. codecov flakiness, but these are becoming less common.

What else besides green checks?

PR descriptions should follow the required template. See CONTRIBUTING and Submitting-PRs. Even though the JIRA link is required, don't expect reviewers to dig through the issue. The description on PR should give any reviewer the context necessary. Screenshots are useful.

Other Suggestions

If your PR is getting big, please consider breaking it up.

While not necessarily part of PR, a README doc for new features would also help during review.

[Brandon32]

There are reasonable times where the PSI check fails and causes PRs to not have all checks passing, When does milo ship below 100LH?, do we need a way to flag that so all checks can be green? It's still useful knowing how bad the score is and using a dummy URL here feels like the wrong approach, will make it harder to verify and bring up questions.

[hparra]

I agree. The dummy is not to skirt a legitimate check. It's a workaround for 2 cases:

1. helix-bot won't check a non-Milo url, e.g. a BACOM url
2. PR doesn't actually involve a change where PSI is required

One assumption is that everyone who reviews a PR is also checking the test urls. It should be obvious if someone is trying to cheat around the check if it's not one of the two cases above.

Ideally, we would modify helix-bot to handle those two cases, e.g. accept other urls for test and maybe support skipping PSI if PR has a certain label or something.

Related: MWPW-130837

[mokimo]

To add to the wonderful list of @hparra

PR Descriptions

Every PR should have a meaningful description on why a change is necessary

Linking a Ticket

Every PR should have a ticket attached to it.

Unit tests

• Every feature, new block, new code should include unit tests.
• Every bug fix should have a new test case ensuring this bug will not pop up in the future again.

IF a unit test is consistently failing and you can't get the check back to passing on your PR

• Ping the developer, to see if it's a false positive or a legitimate issue
• If it's a false positive, either fix or skip the test

Changes related to consumer projects

At a minimum you need to include a testable link for a reviewer on main--milo--adobecom - if you fix an issue related or affecting any consumer project, please include testable links to those as well.

Testing instructions

Include testing instructions on the pull request so that reviewers can test your change without a lot of additional context

[mokimo]

If a test can't pass for any reason, such as a page with target running the LH checks would always fail -> please add some reasoning to the PR on why a check is failing and can be safely merged ignoring a particular check.
This should be an exception