-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Improve error messages from failing tests #8225
Improve error messages from failing tests #8225
Conversation
@@ -280,7 +274,7 @@ var _ = Describe("Podman exec", func() { | |||
exec := podmanTest.Podman([]string{"exec", "-ti", ctrName2, "id"}) | |||
exec.WaitWithDefaultTimeout() | |||
Expect(exec.ExitCode()).To(Equal(0)) | |||
Expect(strings.Contains(exec.OutputToString(), fmt.Sprintf("%s(%s)", gid, groupName))).To(BeTrue()) | |||
Expect(exec.OutputToString()).To(ContainSubstring(fmt.Sprintf("%s(%s)", gid, groupName))) |
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.
Should this equal?
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.
The output of id
would at least also have the UID, so we can't just use Equal
without further changes to the test.
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 am fine with this, then.
Using a function like ContainSubstring or Equal is better because if the test fails it will log a descriptive error that includes the actual string generated during the test. This is more helpful than a function like BeTrue that will only indicate that an assertion failed without giving further details of the failure. Signed-off-by: Debarshi Ray <rishi@fedoraproject.org>
Thanks for the review! |
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.
LGTM
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: debarshiray, rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
Using a function like ContainSubstring is better because if the test
fails it will log a descriptive error that includes the actual string
generated during the test. This is more helpful than a function like
BeTrue that will only indicate that an assertion failed without giving
further details of the failure.
Signed-off-by: Debarshi Ray rishi@fedoraproject.org