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(createtestfromscenario.js): add feature to hide pending scenarios from test run #474

Conversation

rbcasperson
Copy link

First, thank you for the work on this project! I am a huge BDD advocate and practitioner, and to be able to combine Cypress and BDD is a huge win for me. This is my first attempted contribution, but I wouldn't be surprised if I think of new features I'd like, in which case I'm always happy to try to make it work myself.

What

This change allows you to control whether or not you want to hide pending scenarios. If you set the
env var to hide pending scenarios, then any scenario that should not be run will not get a mocha
test case. In other words, only tests that will actually be run will appear in the cypress test run.

Why

If you have a lot of scenarios or features that are skipped, registering them with Cypress can create a lot of noise in the test output, and can even slow down a test run significantly. This allows you to hide those "pending" scenarios from Cypress, if you would like.

In my personal case that drove me adding this change, I have a lot of feature files and scenarios that tagged with a tag that causes them to be skipped. I use feature files to define the overall behavior that needs to be tested, and a lot of those tests are unfortunately still manual. This means I have a lot of scenarios that are skipped, but those may be implemented in due time. So it is important to me to keep all automated and manual test feature files organized together.

When these tests are run in CI however, we run all of the .features files. This picks up all of the features, even if all scenarios are to be skipped by Cypress. That I think is fine behavior, but the issue is that going through skipping all of those tests takes a while in the test run. This is because I have before and beforeEach hooks. The code there is pretty simple, but that still runs the hooks for every skipped feature/scenario. Perhaps that is a bug in and of itself, but that I think is a higher architectural discussion (which we can have if you'd like!). I felt like the approach here was the simplest given how things work now, but I am open to your suggestions on a preferred solution to my problem. If the skipping of the tests took basically no time, that would solve most of my issue, but I also do like the ability to hide the skipped tests from the output altogether, so the CI output is very clean and concise.

Notes on my implementation

As mentioned, I tried to find the simplest solution to the problem I am having. I also looked into the cypress-tags.js code and thought that maybe a solution could be to include logic to handle .features files, but cypress-tags.js really only determines which files to tell cypress to run, whereas the logic to convert the .features file into basically a large feature file is run within the plugin code, so that seemed like a major change to make work.

Putting the change right into the place where it creates the test allows the option to be used regardless of how you trigger cypress, and it is a really simple bit of logic.

I briefly looked into adding tests for this new feature. It doesn't seem there is any test coverage in the scenario creation files, so currently I don't have any test coverage. I'm not immediately sure how I'd approach seeing which tests are or are not created. It probably would make the most sense to test this at a higher level, perhaps actually calling cypress run and looking at the test output. But I am not sure if you want to go down that path. I don't mind spending a little time to help with that if you would like, but it seems out of scope for this change.

I chose to use an environment variable as an option since it is easily available to the code that needs it, and I didn't see any precedent for passing through options to the command line. If you would prefer a command line option, let me know and I can try to make that work.

I have tested this change in my test environment with and without the environment variable set, set to true or false. Not having it set or set to false follows the same current behavior, but setting it to a truthy value makes it as if the scenarios didn't exist in the feature files.

…s from test run

This change allows you to control whether or not you want to hide pending scenarios. If you set the
env var to hide pending scenarios, then any scenario that should not be run will not get a mocha
test case. In other words, only tests that will actually be run will appear in the cypress test run.
@carlesandres
Copy link

Loving this one! It would be great to see it reviewed.

@badeball
Copy link
Owner

badeball commented Feb 5, 2021

I'm opposed to any changes in regards to JSON formatter and would much rather see it go away entirely. Alternatively, someone can take their time and actually spec the desired behavior (I'm not holding my breath on that one).

@badeball
Copy link
Owner

badeball commented Feb 5, 2021

In fact, I'd much rather that #329 be simply reverted until someone can come up with a solution that's not entirely, f* retarded.

@badeball
Copy link
Owner

badeball commented Feb 5, 2021

#503 and cypress-io/cypress/issues/14867 is related to the above-mentioned PR.

@lgandecki
Copy link
Collaborator

I think it's time to remove the reporter. We can either do it by moving ahead with your branch, or by removing it from this codebase for now. We don't use it at TheBrain and none of our customers do as far as I know, it's too complex and causes too many problems. It would be nice to come up with some alternative for all the folks that do use it. But for now they could stick with the versions that work for them.

@lgandecki
Copy link
Collaborator

you can use this to modify your node_modules:
https://www.npmjs.com/package/patch-package

you should be very careful doing so for run-time dependencies (any code actually getting executed in production), but for test/build tooling it's usually ok to tweak it a little.

@badeball
Copy link
Owner

Due to personal reasons, the previous maintainers of this package are stepping down and handing the reigns over to me, a long-time contributor to the project and a user of it myself. This is a responsibility I'm very excited about. Furthermore, I'd like to thank @lgandecki ++ for all the work that they've done so far.

Read more about the transfer of ownership here.

The repository has however moved and all outstanding issues are being closed. This is not a reflection of the perceived importance of your reported issue. However, if after upgrading to the new version, you still find there to be an issue, feel free to open up another ticket or comment below. Please make sure to read CONTRIBUTING.md before doing so.

@badeball badeball closed this Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants