Skip to content
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

Merged
merged 24 commits into from May 6, 2022
Merged

feat: add test.failing method #12610

merged 24 commits into from May 6, 2022

Conversation

michalwarda
Copy link
Contributor

@michalwarda michalwarda commented Mar 27, 2022

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

@facebook-github-bot
Copy link
Contributor

Hi @michalwarda!

Thank you for your pull request and welcome to our community.

Action Required

In 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.

Process

In 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 CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

Copy link
Member

@SimenB SimenB left a 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.

@mrazauskas
Copy link
Contributor

@michalwarda The type tests live here. You can run them with yarn test-types (just remember to build before). Please, do ping me if something is puzzling.

@michalwarda
Copy link
Contributor Author

Sorry had a busy week! I'll try to finish the PR during the weekend or at the start of next week!

@SimenB SimenB added this to the Jest 28 milestone Apr 6, 2022
@zpao
Copy link
Contributor

zpao commented Apr 18, 2022

Poking to trigger CLA status check

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@SimenB
Copy link
Member

SimenB commented Apr 21, 2022

This isn't breaking, so removing it from the milestone

@SimenB SimenB modified the milestones: Jest 28, High priority future Apr 21, 2022
@michalwarda
Copy link
Contributor Author

michalwarda commented Apr 26, 2022

Ok! I've finished adding type specs!
Similarly I've added the docs to changelog. Wish I was faster to land in 28 but I'll do my best to at least land in the next one!

Answering the feedback below:

This all looks great to me, thanks! We're missing type tests, a changelog entry and signing the CLA, hopefully those are quick slightly_smiling_face

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.

I think it's important that a user can skip a failing test.
If ie. someone is trying to debug the failing test because it's passing for some reason then adding a only to it rather than replacing failing with only will keep the behavior as it was. Otherwise it might be a bit confusing.
Similarly because the code in a failing test is "running" then a user might want to add a skip to a failing test because he is debugging another one.

And 1 more thing I noticed is that a new concurrent modifier is being added to the Global.It. I think it is in conflict with my changes. I did not try to make it work with that because I'm not sure how it's supposed to work yet :D

docs/GlobalAPI.md Outdated Show resolved Hide resolved
docs/GlobalAPI.md Outdated Show resolved Hide resolved
@michalwarda
Copy link
Contributor Author

michalwarda commented Apr 26, 2022

And 1 more thing. I'm NGL I have no idea how to implement it in jest-jasmine2. I think I would need an intro to do that

@SimenB
Copy link
Member

SimenB commented Apr 26, 2022

Don't bother with jasmine, it's fine to just handle circus for now 👍

docs/GlobalAPI.md Outdated Show resolved Hide resolved
@michalwarda
Copy link
Contributor Author

michalwarda commented Apr 29, 2022

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 failing or concurrent to tests that might clean up the code because it's quickly getting out of hand how it's being used. I was thinking about using Proxy objects for that. I can prepare an example of that if you're interested in it at all.

@SimenB
Copy link
Member

SimenB commented Apr 29, 2022

Sorry, missed your last push! Triggered CI now, will review tomorrow (CET) 🙂

@SimenB
Copy link
Member

SimenB commented Apr 30, 2022

I can prepare an example of that if you're interested in it at all.

Definitely! A separate PR, but it sounds interesting 🙂

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking really good!

docs/GlobalAPI.md Show resolved Hide resolved
e2e/__tests__/testFailing.test.ts Show resolved Hide resolved
docs/GlobalAPI.md Show resolved Hide resolved
@@ -731,6 +731,60 @@ test.each`
});
Copy link
Member

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

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

Copy link
Contributor Author

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?

Copy link
Member

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?

@SimenB
Copy link
Member

SimenB commented May 2, 2022

@michalwarda ping 🙂 I really wanna get this over the line and release it 😀

@@ -731,6 +731,60 @@ test.each`
});
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. 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.

docs/GlobalAPI.md Show resolved Hide resolved
@michalwarda
Copy link
Contributor Author

michalwarda commented May 5, 2022

@SimenB

@michalwarda ping slightly_smiling_face I really wanna get this over the line and release it grinning

I also wish to do that :D. If I'm not mistaken the last thing left to do is potentially add description about failing for concurrent and decide if we want to go with failing each.

Though for the failing.each part I would wait both from technical perspective, (as I mentioned, I have an idea on how to make it much cleaner to define those "modes" so that it will be more readable), and from documentation perspective I think we would need to decide on how to document those nested modes in a way that failing will not have to be mentioned for each variant.

Copy link
Member

@SimenB SimenB left a 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

SimenB and others added 2 commits May 6, 2022 12:08
Co-authored-by: Tom Mrazauskas <tom@mrazauskas.de>
@SimenB SimenB changed the title Add test.failing method. feat: add test.failing method May 6, 2022
@SimenB SimenB merged commit 34d15d1 into jestjs:main May 6, 2022
@github-actions
Copy link

github-actions bot commented Jun 6, 2022

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Purposefully failing test (test.failing)
5 participants