-
Notifications
You must be signed in to change notification settings - Fork 10.6k
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
Delete end2end nosec tests #28946
Delete end2end nosec tests #28946
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great!
I'd be curious to know what the small number of lines are that we lose coverage on. But unless they're something really significant, I think we should move forward with this.
Thanks! The tools I used to run coverage reports don't lend themselves well to doing coverage diffs (manual sponge executions). I'll take another pass at it using some different tools and see if I can get a better delta. |
I pieced together a system to merge and diff lcov files in a reasonable amount of time without crashing our build servers, 13,000 lcov files > 12GB file upload limit. Sponge coverage does not support one-off diffs. It'll take some more work to get coverage set up systematically so we can see this at a glance, but I see some paths to get there, and we should get there eventually. This exercise compares individual runs of What I found is, I think, mostly coverage that may differ between individual test runs. Of the ~85,000 instrumented lines: 45 lines are exercised in the baseline but not covered hereExamples:
76 lines are exercised in this pr, but not in the baselineExamples:
I have 1MB lcov diff files I can share for both scenarios. But I didn't see anything critical |
@drfloob I'd be curious to know the impact on the total test runtime for the entire C core test suite (e.g. when you run all the tests locally). I suspect there'll be a noticeable improvement in the total test runtime (since you're removing a lot of tests). I also expect a significant reduction in the number of test targets). |
@jtattermusch Yes, this fixes #27733, and unblocks #28667 for the same reasons. The end2end total test count goes down from about 11k to 7k, which cuts a few minutes off of the total runtime for internal test jobs (600 running in parallel). Do you want to take any other baseline measurements before I land this? |
@jtattermusch Actually I realize you just finished collecting a large swath of metrics, so I think we should be good to go for this PR. |
After removing insecure builds, (internal) coverage results now show next to no changes after removing all of the nosec end2end tests.