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

Create upper and work directories in new locations if sharing scratch #929

Merged
merged 1 commit into from
Jan 27, 2021

Conversation

dcantah
Copy link
Contributor

@dcantah dcantah commented Jan 25, 2021

Signed-off-by: Daniel Canter [email protected]

@dcantah dcantah requested a review from a team as a code owner January 25, 2021 17:56
@@ -206,10 +206,10 @@ func Test_RunContainer_VirtualDevice_GPU_LCOW(t *testing.T) {

testDeviceInstanceID, err := findTestNvidiaGPUDevice()
if err != nil {
t.Fatalf("skipping test, failed to find assignable nvidia gpu on host with: %v", err)
t.Skipf("skipping test, failed to find assignable nvidia gpu on host with: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this was intentional was it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was, shouldn't we be doing what it says in the description and skipping instead of failing the test?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, these were changed specifically to Fatal since we now use features to determine what tests to run, so these would only be run intentionally. In which case, they should fail if the machine doesn't have the correct resources.

It sounds like instead the comments should be updated to reflect that. But either way, that should be done in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok gotcha the comment was tripping me up. I'll revert these!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up fixing some of the ones that actually were Skip, and just changed ones that didn't have any formatting needed to just Fatal from Fatalf

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.

3 participants