-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
feat: add test.failing
method
#12610
Conversation
Hi @michalwarda! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
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.
This all looks great to me, thanks! We're missing type tests, a changelog entry and signing the CLA, hopefully those are quick 🙂
However, as you mention in #4627 (comment), this doesn't use TestMode
. I don't really understand why it's not possible to do so now? I don't think we need only
and skip
modifiers on a .failing
test - people should just change the modifier in that case.
@michalwarda The type tests live here. You can run them with |
Sorry had a busy week! I'll try to finish the PR during the weekend or at the start of next week! |
Poking to trigger CLA status check |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
This isn't breaking, so removing it from the milestone |
Ok! I've finished adding type specs! Answering the feedback below:
I think it's important that a user can And 1 more thing I noticed is that a new |
And 1 more thing. I'm NGL I have no idea how to implement it in |
Don't bother with jasmine, it's fine to just handle circus for now 👍 |
Hey! Not sure about what should I do now, if we're waiting for a good moment for merge then cool! If you need me to do anything else about the pr hit me up with the request :) and I'll clean it up more :). I have my weekend empty and I can probably manage to fit in some time for changes :) Also, if you're open to it I was thinking about a potential restructuring to the whole binding of flags like |
Sorry, missed your last push! Triggered CI now, will review tomorrow (CET) 🙂 |
Definitely! A separate PR, but it sounds interesting 🙂 |
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.
looking really good!
@@ -731,6 +731,60 @@ test.each` | |||
}); |
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.
oh, and test.concurrent.failing
should work
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.
Ok. I think I've managed to cover this! Though I'm not sure how to document it because it will be at least another few cases in README. test.concurrent.failing
, test.concurrent.skip.failing
, test.concurrent.only.failing
.
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 I add all of those? If so do we want it to say like Works like test.failing but concurrently
? Or a whole copy of description?
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 agree we should refactor these docs to not mention every variant, but rather which modifiers are compatible with each other. Not sure how to best do that though, maybe we can leave it for a follow-up?
@michalwarda ping 🙂 I really wanna get this over the line and release it 😀 |
@@ -731,6 +731,60 @@ test.each` | |||
}); |
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.
Ok. I think I've managed to cover this! Though I'm not sure how to document it because it will be at least another few cases in README. test.concurrent.failing
, test.concurrent.skip.failing
, test.concurrent.only.failing
.
I also wish to do that :D. If I'm not mistaken the last thing left to do is potentially add description about Though for the |
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.
thank you! I think the issue this fixed is almost the first one I submitted 😅 Very happy to see it land!
I think we can keep docs tweak for later
Co-authored-by: Tom Mrazauskas <tom@mrazauskas.de>
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
This fixes #4627. In certain scenarios ie. when writing code in BDD way developers would like to write down a more broad test that fails purposefully, and later as soon as it passes it should inform that it can now be marked as not failing anymore.
Feature itself is heavily inspired by RSpec pending tests.
Test plan
Added both unit and e2e tests for the feature. Everything should be covered :).