-
Notifications
You must be signed in to change notification settings - Fork 67
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
Global coverage vs local package coverage. #119
Comments
My initial thoughts on a global config is that if a team is aware of a lower coverage level they could set, they may protest for control so they don't have to meet the coverage. Way around this is to set it to what the tollgates currently require (assuming we can't argue with tollgate coverage requirement) |
Marking this for later rumination, good points. |
I think the central point that @kknoerz brought up in our Monday planning was that "teams should not be able to impede other teams through low coverage of their own packages". Maybe that will be sorted out by stopping projects that are imported into a Modular repository from being PR'd into Is that it? We could also use Danger to report on the coverage of each package, if that would help? I think the next steps were that @Cono52 was going to experiment/spike mapping During a discussion with @NMinhNguyen he mentioned to me that "finding affected packages if you run Jest per package might be tricky" and I agree. I was wondering whether it would be possible to run Jest against the whole project, but then to have some logic which chunks tests results by package paths, and then use these chunks when caching / reforming test results or producing individual or global coverage reports. |
@Cono52 assigning this to you, let's discuss during planning on how to take this forward. |
As discussed in planning, until after modular 1.0, we will enforce an above 70% and over code coverage globally (as oppose to per package/module or per file coverage ), in accordance with the current NIO rules. |
Since adding my partial test change back to Credit, times have improved a lot, (a recent build took 12mins as oppose to 1h 30mins) but now instead of reporting an average coverage in the changed package, we now report the average coverage of the changed file(s).
First complaint from someone with a failing build, "but I didn't create this file, I just needed to make a small change, now you say I have to get the coverage above 90%?!?!"
Devs were relying on these averages by testing some files really high (above 90%) so they could get around other files (probably ones they find/are actually tricky to test).
Now, for example, if they have a branch with a change to just this uncovered file, it will fail against the global jest.config.js coverage threshold.
With this in mind and the Sonar requirements for a repo:
Sonar only supports a coverage at the repo level.
50% coverage report for a develop/master.
70% on 90 day old code and 50% on rest.
I imagine in the short-term we will need a global jest to be sure we meet the above requirements with no single package dragging the rest down.
@NMinhNguyen pinged me some threads that will help the discussion here:
nrwl/nx#622
jestjs/jest#2418
Quote form the nx issue: "Having one report doesn't really work for most organizations. Imagine you have two teams building two apps in the same repo. The fact that one team failed to meet a certain threshold shouldn't affect the other team.
So we can have a report per team (app+all its libs), but then the situation tricky if you have a lot of shared libs."
In relation to above, we are stuck at the moment with one report and if we enforce a global level coverage prior to PR merge ,teams can't impact each other.
So, I was wondering if we have already discussed ideas around this? and if not, maybe we could bake something in here so projects in the repo are certain to meet the tollgate requirements.
The text was updated successfully, but these errors were encountered: