chore: remove e2e_slurm_misconfigured series tests #10023
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rb-determined-ai
requested review from
ShreyaLnuHpe,
gt2345 and
davidfluck-hpe
✅ Deploy Preview for determined-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
rb-determined-ai
force-pushed
the
rb/rm-znode-ch2
branch
from
d46c759 to
fa8c2d3
Compare
rb-determined-ai
requested review from
dannysauer,
stoksc and
azhou-determined
and removed request for
gt2345,
davidfluck-hpe and
ShreyaLnuHpe
rb-determined-ai
force-pushed
the
rb/rm-znode-ch2
branch
from
fa8c2d3 to
6f45ff1
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10023 +/- ##
==========================================
+ Coverage 54.62% 54.65% +0.02%
==========================================
Files 1260 1260
Lines 157558 157556 -2
Branches 3632 3630 -2
==========================================
+ Hits 86071 86108 +37
+ Misses 71353 71314 -39
Partials 134 134
Flags with carried forward coverage won't be shown. Click here to find out more.
|
azhou-determined
approved these changes
Oct 4, 2024
The only test in this series was an e2e test that the log shipper ships a certain log in a certain situation. We have unit tests for that, so delete the e2e test. Now, I actually failed this e2e test when I first landed the log shipper, so it wasn't totally good-for-nothing. But effectively, all it was testing these days was that we did in fact use the new log shipper in slurm workloads. Now that the EE code is merged into the OSS code, and we can just use the same LogShipperWrappedEntrypoint() function in all three classes of resource manager, I don't see any real justification for this test. This is part of a larger effort to get rid of our znode tests, which are notoriously unreliable.
rb-determined-ai
force-pushed
the
rb/rm-znode-ch2
branch
from
6f45ff1 to
c3b8921
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The only test in this series was an e2e test that the log shipper ships
a certain log in a certain situation.
We have unit tests for that, so delete the e2e test.
Now, I actually failed this e2e test when I first landed the log
shipper, so it wasn't totally good-for-nothing. But effectively, all it
was testing these days was that we did in fact use the new log shipper
in slurm workloads.
Now that the EE code is merged into the OSS code, and we can just use
the same LogShipperWrappedEntrypoint() function in all three classes of
resource manager, I don't see any real justification for this test.
This is part of a larger effort to get rid of our znode tests, which
are notoriously unreliable.