-
Notifications
You must be signed in to change notification settings - Fork 15
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
Enforce expectation names uniqueness in CheckResultOutline test #1549
Conversation
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.
Yeah it's a lot more consistent this way :D
LGTM :D
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 but since those are strings, could we just use uuids?
b08c9d2
to
9bdb891
Compare
const expectationName3 = faker.color.human(); | ||
const expectSameExpectationName1 = faker.lorem.word(); | ||
const expectSameExpectationName2 = faker.lorem.word(); | ||
const expectationName1 = faker.datatype.uuid(); |
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.
@dottorblaster I applied the suggested change about uuids.
I am afraid, though, that the should expectation names be uuids? doubt might rise, which is not the case.
As long as it clear that we just want strings that are unique in a restricted context and not necessarily uuids, we should be good.
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.
Yeah I agree, for me it's clear, I don't know if you want to leave a comment or something 😅
9bdb891
to
0e14eae
Compare
0e14eae
to
6e8074a
Compare
Description
This should remove flakiness for the specific test changed.