Skip to content

Refactor system/dist/CUDA tests #2382

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

trxcllnt
Copy link
Contributor

This PR extracts the test changes from #2356 per @drahnr's request.

The system and dist tests now use an SccacheClient instance (a519fc3), which is a struct that wraps an sccache client process. Each SccacheClient instance starts its sccache client on a distinct port, and shuts it down when SccacheClient dropped.

With this improvement, system (and dist) tests can now run in parallel. Since each test creates its own isolated SccacheClient instance, no need to carefully manage global start_local_daemon(), stop_local_daemon(), zero_stats() etc. calls.

The second commit (b307d9a) refactors the CUDA tests to be easier to read and update. Rather than individually asserting various struct fields match exact values, it's easier to create a ServerStats instance, increment the values I expect to change, and compare against the real stats.

@trxcllnt
Copy link
Contributor Author

The failing nvcc tests on Windows will be fixed by bug fixes in the follow-up PR.

@codecov-commenter
Copy link

codecov-commenter commented Apr 15, 2025

Codecov Report

Attention: Patch coverage is 90.08264% with 144 lines in your changes missing coverage. Please review.

Project coverage is 70.90%. Comparing base (9fb942e) to head (5d43c0c).

Files with missing lines Patch % Lines
tests/system.rs 87.30% 98 Missing ⚠️
tests/harness/dist.rs 93.96% 26 Missing ⚠️
tests/harness/client.rs 85.92% 19 Missing ⚠️
tests/dist.rs 99.01% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2382      +/-   ##
==========================================
- Coverage   71.41%   70.90%   -0.52%     
==========================================
  Files          65       67       +2     
  Lines       36349    35623     -726     
==========================================
- Hits        25960    25257     -703     
+ Misses      10389    10366      -23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sylvestre
Copy link
Collaborator

i am not comfortable landing a change that would deliberately break the CI :/

@trxcllnt
Copy link
Contributor Author

@sylvestre #2383 branches from this PR, I just split them up per a request from @drahnr.

#2383 has both sets of changes (diff of the two branches here), so it's fine to ignore this one and merge #2383 instead.

I will also be adding nvcc --device-debug and nvcc binary compilation support in separate follow-up PRs, since those are very minor changes compared to these.

@trxcllnt
Copy link
Contributor Author

cc: @robertmaynard for review

@drahnr
Copy link
Collaborator

drahnr commented Apr 22, 2025

My suggestions was to split out the structural test changes, I didn't say broken CI would be acceptable :) If there are specific tests that would be fixed later on, please defer them to the relevant other PR(s).

@trxcllnt
Copy link
Contributor Author

The test improvements exposed an underlying bug, because now they compare fields that weren't compared before. I apologize if that wasn't clear before.

The test changes are isolated to this PR to make them easier to review, but they are also included in the PR (#2383) that includes the fix. The diff of the test changes and the fix is here.

The changes to the tests can be merged separately from the bug fix by merging this PR, then immediately merging #2383, or they can be merged together by closing this PR and only merging #2383.

Let me know if there's anything I can do to make reviewing these easier.

@sylvestre sylvestre force-pushed the fea/enhance-system-tests branch from b307d9a to 2470bab Compare April 25, 2025 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants