-
Notifications
You must be signed in to change notification settings - Fork 97
fix: testLifoStorage CI failures #3387
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3387 +/- ##
===========================================
+ Coverage 56.33% 56.42% +0.08%
===========================================
Files 1065 1065
Lines 90242 90396 +154
===========================================
+ Hits 50836 51003 +167
+ Misses 39406 39393 -13 ☔ View full report in Codecov by Sentry. |
rrsettgast
left a comment
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.
I suggest just commenting out the test, including a comment indicating the problem, and the situations that this tests is providing coverage for.
|
@wrtobin Do you have any comment on this? I think it still needs to be fixed, but this functionality is for the Makutu team, so it won't affect the rest of the code. |
|
I agree that commenting out and documenting the test (with a link back to this MR or an open issue) would be fine here. We still want it to be fixed of course, but since the FIFO storage stuff is only used for the Makutu team it should be fine. One would hope that in use the FIFO storage wouldn't use size-0 buffers on device (since that seems counter to the intent of the container), though having that case not be catastrophic would certainly be preferable. Further, if the intent in practical use is actually never to have a size 0 buffer on device with the LIFO container, we could add a runtime check and error out if it actually happens. |
|
I'm going to merge this since it will help gain some time in handling the merge queue (it will avoid some CI re-runs, hopefully) |
* temporarily removed offending test * commented test instead of removing, added explanation * uncrustify
The
testLifoStorageunit tests still occasionally fail (issue #3355).After #3362, fails seem to only happen in the
LifoStorageBufferOnCUDANoDeviceBuffertest. This is a corner case as it uses a zero-size device buffer which might be buggy.To help smooth the CI process, I propose disabling this specific test for now, pending further review.