Skip to content

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
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)

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?

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