MB-16176 - Create coverage ADR

[ghost]

This PR should create an ADR that suggests improvements to the existing code test coverage checks with the intention to avoid PR roadblocks and make the checks more transparent.

[kctruss]

typo: waintain => maintain

[ghost]

Thanks for catching this -- we've got to waintain our spelling standards too.

[kctruss]

I'm not sold on the Happo approach here. My concern is that rather than alleviate a problem, this approach shifts the responsibility from the engineer doing the work onto the PR review process, which is already a bottleneck.

Personally, I'd rather see us spend time documenting and coaching on when to stop fighting the test results and communicate a cache reset - it should be less "break glass in case of emergency" and more "Hey, here's the situation".

[rogeruiz]

Leaving some comments and suggestions here. I think we should have another option that I mention below. This is in-line with @kctruss feedback about treating resetting the cache as a thing that could happen instead of something that should only rarely happen.

[rogeruiz]

Are these the only scenarios we'd like to discuss here? I think these are enough, but I'd like to hear if folks can think of another or perhaps expand on these scenarios a bit more with more examples.

[felipe-lee]

thought: I think another example for scenario 3 would be adding logic to existing model_to_payload functions since those aren't unit tested and are only sometimes integration tested via the handler integration tests.

[rogeruiz]

This link should also point to documentation around the coverage tests and how to increase it. I think it's also possible to include this as a third option that may be the best of keeping the tests and having a GH Action that alerts authors to read the specific documentation about how to interpret the code coverage results for the client and the server and also reset the cache in a way that can be tracked / discussed when it comes up.

[felipe-lee]

thought: +1 to the link to docs and that this could be its own third option, even if we are already implementing the happo option.

[ghost]

That makes sense to me -- I'll gauge folk's feelings at the FE/BE check-in and update the ADR accordingly.

[YanZ777]

I think another few options would be to

• maintain current test coverage and create documentation around resetting the cache
• lower the test coverage number
• review current test coverage and create tickets that increases the test coverage

I also feel a little uneasy with going down the Happo Approach as I worry about that becoming something that folks ignore.

[ahobson]

Do we have any data for how often scenario 1 and 3 happen? In my memory, I can recall it happening twice a year which to me doesn't suggest we really have a problem. We have hundreds of PRs a year that the current process ensures we don't have a coverage decrease. Making a change for such an infrequent event doesn't really make a lot of sense to me, personally.

[ahobson]

I'll also say that I've seen the test coverage problem come up many times, and most of the time the problem is that folks haven't rebased their PR or they need to increase test coverage. IMHO, we should just get rid of the coverage check instead of making it optional, as I believe folks will just ignore it.

[rogeruiz]

@ahobson Thanks Drew. Is this supposed to read "should just get rid of" or "shouldn't just get rid of"?

[ahobson]

@rogeruiz IMHO, if we think the test coverage problem is major enough to make the checks optional, we should just remove the checks entirely.

[ahobson]

I've been thinking about this, and I think I'm wrong. I don't have any data to support my position that making the check optional will actually decrease test coverage.

This proposed change is one that we could easily undo if we needed to.

How about this: We note the coverage before putting this change in, and then one month later, at BE/FE check in, we report on the coverage numbers and how they compare to before the change. We can this discuss in the check in if the changes are concerning or not and if we want to keep the checks optional. It can become something we bring up monthly at the check in to ensure we aren't drifting the wrong way.

Thoughts?

[ahobson]

I updated the PR to record the coverage on merge to main to the govdev environment and we now have a dashboard.

[ghost]

@ahobson I think this plan makes a lot of sense -- I can mention in the chosen option that we will review it after a month. I should also note that a fairer comparison would be with the coverage if we didn't make the check optional (which we can't know), which would also potentially decrease every time we manually reset it.

[felipe-lee]

thought: Good point @michaelvaldes-truss. I wonder if to make that comparison match better we could compare the new change in test coverage against historical data of how coverage has changed previously with the required check in place since we have reset the cache a few times.

[ghost]

I also want to address the very valid concerns expressed in comments on this PR -- that making the check optional would make it too easy for coverage to decrease. My assessment is that the current strategy already allows for decreases when folks manually reset the limit, and this can happen for very valid reasons (scenarios 2 and 3 described in the ADR). Analyzing coverage decreases is difficult to do automatically, so the intention of the Happo Approach was to put it entirely in the hands of the PR reviewers and also make it easier to access information relevant to that analysis.

I added these points to the ADR and also introduced a third option, Lenience for Tiny Coverage Decreases, which is a middle ground that attempts to partially address these concerns.

@YanZ777 @kctruss

[ruizajtruss]

I do want to note that while I support the third option presented - there's a small chance there could be a .2% difference if you pull main to fix a .1% diff in coverage. That's what happened to me this last go round with coverage issues. Little more detail in this thread for anyone curious.

[YanZ777]

I've thought about another option to consider, and that's to see if we can just have PRs run test coverage only on new files.

I think regardless of whichever option gets picked, we should analyze which packages have the lowest coverage below a certain threshold and write up tickets so that tests can be added to increase the coverage.

[YanZ777]

question: Will this ADR be reassessed in the future? Is the plan to have the test coverage be optional forever?

[felipe-lee]

It seems like maybe we'll re-assess after a month to see if things are going well: https://github.com/transcom/mymove-docs/pull/350/files#r1221538465

Though if things aren't going how we'd like them to, we'd likely then have to decide how we want to correct things.

[ghost]

Thanks -- I added a note in the ADR to reflect this!

[ruizajtruss]

I approve in support of the third option presented

[mandaem]

I liked the Happo Approach best

[YanZ777]

I think the middle ground is reasonable for the time being.
I would like further discussion on what Kyle mentioned in the BE / FE checkin, which is assessment on if this test coverage check is valuable or not in the first place.
If we do decide that it is valuable to have, I would also like us also consider adoption of a long term strategy of creating a Jira epic ticket to increase coverage for code that doesn't have good test coverage.

[ahobson]

As a datapoint, in the week that we have had test coverage checks disabled

check	2024-06-09	2024-06-16
client	73.7	74.0
server	77.7	77.6

[mmayo-truss]

I appreciate your work on this ADR and the conversation that it has encouraged. I approve this in favor of the "Happo Approach." I think that makes the most sense based on the discussion I have heard.