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

Delete end2end nosec tests #28946

Merged
merged 10 commits into from
Mar 7, 2022
Merged

Delete end2end nosec tests #28946

merged 10 commits into from
Mar 7, 2022

Conversation

drfloob
Copy link
Member

@drfloob drfloob commented Feb 23, 2022

After removing insecure builds, (internal) coverage results now show next to no changes after removing all of the nosec end2end tests.

@drfloob drfloob added the release notes: no Indicates if PR should not be in release notes label Feb 23, 2022
@drfloob drfloob changed the title [WIP][NO_REVIEW] Delete end2end nosec tests Delete end2end nosec tests Mar 4, 2022
@drfloob drfloob requested a review from jtattermusch March 4, 2022 00:07
@drfloob drfloob marked this pull request as ready for review March 4, 2022 00:08
Copy link
Member

@markdroth markdroth left a 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.

include/grpc/event_engine/event_engine.h Outdated Show resolved Hide resolved
@drfloob
Copy link
Member Author

drfloob commented Mar 5, 2022

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.

@drfloob
Copy link
Member Author

drfloob commented Mar 6, 2022

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 end2end:all and therefore it may not be perfectly accurate (some test runs may exercise different retry paths, spurious error paths, etc). It's about an hour of manual work to generate these results, so I think this will have to suffice for now. More broadly, end2end tests only cover ~45% of src, so we should monitor and improve coverage generally anyhow.

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 here

Examples:

  • src/core/lib/transport/metadata_batch.h.gcov.html: a no-op method EncodeTo is called in the baseline, but not in this PR
  • src/core/lib/surface/completion_queue.cc.gcov.html: CheckReadyToFinish is evidently exercised in the baseline, but not in a single run of end2end tests via this PR.

76 lines are exercised in this pr, but not in the baseline

Examples:

  • src/core/ext/transport/chttp2/transport/chttp2_transport.cc: no unseen streams to cancel
  • src/core/lib/iomgr/ev_poll_posix.cc: whether notification is requested after shutdown,
  • src/core/lib/iomgr/tcp_posix.cc: code to handle a recvmsg failure

I have 1MB lcov diff files I can share for both scenarios. But I didn't see anything critical

@jtattermusch
Copy link
Contributor

jtattermusch commented Mar 7, 2022

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

@drfloob also, can you confirm that this PR will solve #27733?

@drfloob
Copy link
Member Author

drfloob commented Mar 7, 2022

@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?

@drfloob
Copy link
Member Author

drfloob commented Mar 7, 2022

@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.

@drfloob drfloob merged commit 762cde4 into grpc:master Mar 7, 2022
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bloat/none imported Specifies if the PR has been imported to the internal repository lang/core perf-change/none release notes: no Indicates if PR should not be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants