-
Notifications
You must be signed in to change notification settings - Fork 273
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
test(defined): create tests for defined() #7967
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.
It's probably not a big problem, but if we don't squash the commits then on the first commit the test description lines 10 and 14 don't describe the test correctly.
Yes, in the first commit the comment doesn't match the test. It's actually to validate the current behavior of defined() so that the test passes, even though this behavior doesn't correspond to what is really expected (hence the fix). But I can change it if it's an issue. |
I don't know if it's worth changing or not, let's get @fbeauchamp's opinion. |
3c04b83
to
aa3b4be
Compare
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.
Please check in the github CI that theses tests are really ran
(last run is :
https://github.com/vatesfr/xen-orchestra/actions/runs/10778481900/job/29889906516 and I don't see defined here since the package.json entry is missing )
f6caca5
to
f4b1712
Compare
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.
PLease rebase so that w have one commit for the fix, on one for the test
f4b1712
to
a96b4e5
Compare
a96b4e5
to
e11b313
Compare
Review by commits, DO NOT SQUASH.
Create tests for defined()
issue XO-163