Cypress-cucumber-preprocessor: After method is not being called if one of my steps failed

Created on 26 Jun 2020  ·  21Comments  ·  Source: TheBrainFamily/cypress-cucumber-preprocessor

Current behavior

When all the steps passed then the After method got called but when one of the step failed the After method was never called.

Desired behavior

The After method contains my cleanup code for the test so I would like it to be executed.

I'm using Cypress, Typescript, Cucumber and Webpack combination.

Versions

  • Cypress version: 4.9.0
  • Preprocessor version:
    "@cypress/webpack-preprocessor": "5.4.1"
    "cypress-cucumber-preprocessor": "2.5.0"
    "typescript": "3.8.3"
    "webpack": "4.41.0"
  • Node version: 12.16.1

Most helpful comment

I haven't looked at that one at all to be honest but there are questions from Badeball there that seem unanswered, so I thought about directing my attention to other areas for now.

@lgandecki Hey Łukasz,

What are the unanswered questions? I don't see comments from Badeball on this thread. This is a show stopper for us, I know you suggest that we call something that will restore our system to its initial state but that is not feasible in our case. Our application is run on top of 8-16 servers so we don't have a reliable/timely way to restore all the applications that we're built on top of. Before our suites are run, each suite has to create 8-16 VMs that our app runs on and then make a number of API calls (different per suite) to configure them for different scenarios.

We are not a single server app that can easily have its db re-seeded before each test, but we do know what each test modifies, so undoing what a test has done is the only way for us to be able to reliably run tests over and over after they fail. Right now we have to start another job to get a fresh testbed, which can take up to two hours making it really hard to work on tests that are currently failing.

I don't understand why you'd offer an @After() and then not be willing to take a PR that makes it work. Are you saying that if an app cannot restore its state on a @Before() that Cypress may not the right tool for testing it?

It's frustrating to have you say

Most importantly - Yes we would definitely take a PR with a fix. You can look at resolveStepDefinition.js hookRegistry.js.

And then have the PR rejected without any real explanation.

All 21 comments

I'm also experiencing this issue.

Same problem

Any updates on the issue? This is currently making our testing pretty hard to mantain.

Can you do the cleanup on Before?
https://docs.cypress.io/guides/references/best-practices.html#Using-after-or-afterEach-hooks

From cypress docs:
image

Obviously we might have a bug here, but it might not be a game stopper for anyone. Not sure if we even should have an After hook

Hello Łukasz!

The problem with using Before to clear the state is that our application has a lot of complexity, relations between entities etc. Our environmets are even deployed pre-poluated in order to execute tests as our applications is on top of three other really complex ones.

"The only times you ever need to clean up state, is if the operations that one test runs affects another test downstream. In only those cases do you need state cleanup." this is basically the case for all our tests and we can't rely on order of execution since we would like to run tests in parallel. If we have to create one clean function it will have to take care of every possible failure during any of the steps, because creation and management of a given object often relies on at least a couple of more created in advance during the before phase using API calls.

I see the benefits of running the cleanup during the Before phase, but unfortunately I don't think it is sustainable for large scale applications. If someone ran into the same problem, please share a possible solution :)

Thanks for the explanation. :)
But, still, I'm not sure I understand the situation correctly.
How can you be running tests in parallel if one test can mess up a state for a different one? It seems to me that if you ned to rely on cleaning after each test then the tests would have to be run one by one as a direct consequence of that.
I think the idea of a before cleanup is to reset the DB state to a place from which all of the tests start with. (obviously you can have additional set up for a given test, but all setup functions would start from the same baseline).

If your that baseline is large (gigabytes of data or something like that, or even hundreds of megabytes), then I can see how this might not be practical, especially if you are running things in parallel on the same system.

Not to go too philosophical in a tool that tries to be non-opinionated, but I guess it might be worth considering what are you guys trying to test. I wouldn't put too many things into e2e tests. Just a few (10? 50? but not 100s) scenarios that:
a) cover the most critical happy paths
b) verify the integration of all the systems (for example - did my frontend app deployed correctly and is reachable under a domain, is that app pointing to a correct middle-layer, and the middle-layer connected to microservices, are the microservices connecting to their dbs, is my authorization/authentication workflow working, etc, etc). It's a huge win if you can get that feedback on every push to a repository and on deployment to prod.

The rest of the testing of the frontend can be done with the backend mocked (with for example something like this https://mswjs.io/ - they use cypress around their documentation) , the testing of the backend can be done in the backend itself, and then the contract between can be tested with types (let's say based on GraphQL schema or something like openAPI).

I understand it might not be one person to decision to go one way or another with testing approach, and all what I said here does not excuse the tooling to have a buggy behavior in something that's available as part of it's API ;) so I'm going a bit off topic here.

Can you do the cleanup on Before?
https://docs.cypress.io/guides/references/best-practices.html#Using-after-or-afterEach-hooks

this is probably fine as a workaround. but it's not very reasonable to have a feature file perform the teardown of say persisted data that have nothing to do with the feature being tested. ideally teardown of these objects would occur in context (ie in the feature file that created them) – preferably in an After that always runs.

Thanks for the explanation. :)
But, still, I'm not sure I understand the situation correctly.
How can you be running tests in parallel if one test can mess up a state for a different one? It seems to me that if you ned to rely on cleaning after each test then the tests would have to be run one by one as a direct consequence of that.
I think the idea of a before cleanup is to reset the DB state to a place from which all of the tests start with. (obviously you can have additional set up for a given test, but all setup functions would start from the same baseline).

If your that baseline is large (gigabytes of data or something like that, or even hundreds of megabytes), then I can see how this might not be practical, especially if you are running things in parallel on the same system.

Not to go too philosophical in a tool that tries to be non-opinionated, but I guess it might be worth considering what are you guys trying to test. I wouldn't put too many things into e2e tests. Just a few (10? 50? but not 100s) scenarios that:
a) cover the most critical happy paths
b) verify the integration of all the systems (for example - did my frontend app deployed correctly and is reachable under a domain, is that app pointing to a correct middle-layer, and the middle-layer connected to microservices, are the microservices connecting to their dbs, is my authorization/authentication workflow working, etc, etc). It's a huge win if you can get that feedback on every push to a repository and on deployment to prod.

The rest of the testing of the frontend can be done with the backend mocked (with for example something like this https://mswjs.io/ - they use cypress around their documentation) , the testing of the backend can be done in the backend itself, and then the contract between can be tested with types (let's say based on GraphQL schema or something like openAPI).

I understand it might not be one person to decision to go one way or another with testing approach, and all what I said here does not excuse the tooling to have a buggy behavior in something that's available as part of it's API ;) so I'm going a bit off topic here.

We actually have a long time history with mocking the backend in our tests and we wanted to specifally avoid it. But anyway I agree with your point, but for me it really comes down to the complexity of the app and the unpredicatability a failed test can cause since we have no way to know what exactly should be cleaned in a really complex setup done in the before of the test. We might look for a way to contribute to fixing this, it can't hurt if it works as expected :)

@lgandecki There's a paragraph from the best practices you mentioned that is worth pointing out:

If the state you are trying to clean lives on the server - by all means, clean that state. You will need to run these types of routines! But if the state is related to your application currently under test - you likely do not even need to clear it.

That's the cleanup we are doing. Before a test, we may need to run an API to create backing data required to run a test. For example, a UI test that creates a group must first have users. After the test is run, I want to run commands to delete the users I created in a Before and the group I created in my Cypress test.

I appreciate some of your tips about best practices and I assure you we are creating e2e tests for critical paths. We just have a gigantic application with over 100 discrete workflows that we want to be sure don't regress.

It's unclear to me whether you would accept a PR to fix this problem, based on your comment:

Obviously we might have a bug here, but it might not be a game stopper for anyone. Not sure if we even should have an After hook

Can you please let us know if you'd accept a PR and maybe any guidance into where to start looking?

Most importantly - Yes we would definitely take a PR with a fix.
You can look at resolveStepDefinition.js hookRegistry.js.

as for a philosophical discussion.. :)

I'm still not sure why can't you reset the state before the tests though. I imagine a before step that brings the state of the app to the same "fresh" state. You don't need to worry about removing users you added. The problem with arbitrary after steps that clean things that you've done over the test might be problematic - your test might actually change state in ways you didn't plan for and subsequently breaking other tests. Worst case scenario if your tests run in random order (for example because of parallelization), those errors will be random and extremely hard to track.

In all honesty we do have a large legacy project where we do use after hook to clean the DB changes (not with cypress, that was years before cypress was made public), but we had to program in a "tracker" that hooked into our database, recorded all changes, and in the end reverted them. Without that the tests were flaky. Now that I think about it - we could as well just run the same logic in before step :)

@lgandecki We're taking a look to see if we can fix it. Hopefully we can provide a PR.

A little more on the philosophical discussion, since you don't seem to mind 😅

I understand the arguments Cypress had made for cleanup before tests. However, for cases where it's not as simple as a db.reseed(), cleaning up after yourself does give the tests the properties we're all looking for:

  • They can run in any order
  • They can be parallelized
  • Same test can be run over and over

I understand that tests can leave some state behind because we may miss something, I guess we just haven't had that problem as much. We definitely cannot afford to recreate an environment since it involves somewhere between 8-16 servers working together so we can test different configurations . The script to run that takes somwhere between 20-30 minutes.

If we can't come up with a working PR, we'll have to find some way to revert any test to a known state before a test. We thought of tracking all POSTs and calling DELETE on them, but we have cases where things must first be stopped or disabled before they can be deleted.

The main philosophical argument is that standing up a clean environment before each test is not really feasible for enterprise applications and coming up with some hack to restore its state before each test is not straight forward. Cleaning up after each test, because you know what was done in the test, should be seen as a valid alternative when db.reseed() is prohibitive.

@lgandecki would you mind having a look at https://github.com/TheBrainFamily/cypress-cucumber-preprocessor/pull/538?

Hopefully I will be able to provide another commit if you think It can be improved.

I haven't looked at that one at all to be honest but there are questions from Badeball there that seem unanswered, so I thought about directing my attention to other areas for now.

I haven't looked at that one at all to be honest but there are questions from Badeball there that seem unanswered, so I thought about directing my attention to other areas for now.

@lgandecki Hey Łukasz,

What are the unanswered questions? I don't see comments from Badeball on this thread. This is a show stopper for us, I know you suggest that we call something that will restore our system to its initial state but that is not feasible in our case. Our application is run on top of 8-16 servers so we don't have a reliable/timely way to restore all the applications that we're built on top of. Before our suites are run, each suite has to create 8-16 VMs that our app runs on and then make a number of API calls (different per suite) to configure them for different scenarios.

We are not a single server app that can easily have its db re-seeded before each test, but we do know what each test modifies, so undoing what a test has done is the only way for us to be able to reliably run tests over and over after they fail. Right now we have to start another job to get a fresh testbed, which can take up to two hours making it really hard to work on tests that are currently failing.

I don't understand why you'd offer an @After() and then not be willing to take a PR that makes it work. Are you saying that if an app cannot restore its state on a @Before() that Cypress may not the right tool for testing it?

It's frustrating to have you say

Most importantly - Yes we would definitely take a PR with a fix. You can look at resolveStepDefinition.js hookRegistry.js.

And then have the PR rejected without any real explanation.

@juanmendes we just ran into a similar issue. deleting the entire db in a before hook was causing issues. So i thought about doing the same thing (deleting just what was created). But you can't know what was created until the after hook. And for us it wouldn't work, because these tests were failing and needed to be retried, so the after hook wasn't going to run.

@juanmendes we just ran into a similar issue. deleting the entire db in a before hook was causing issues. So i thought about doing the same thing (deleting just what was created). But you can't know what was created until the after hook. And for us it wouldn't work, because these tests were failing and needed to be retried, so the after hook wasn't going to run.

We considered an approach where we always create new objects during tests, never modify existing objects. We would then serialize all hrefs that were created during a test and delete them in a global @Before(). That is not foolproof and will require a lot of API calls before a test is run. We would rather have tests know how to clean up after themselves and would like some resolution on this issue to decide if we'll be able to keep using Cypress because our tests are unwieldy as of now.

@juanmendes Sorry for that. In the PR there were questions from badeball -
image

They were answered since then. Could you possibly try that version of the code and see if it works the way you'd expect?
@MateuszNowosad this is very important, please take a look at the PR, test it, ask questions, try to break it. If we could merge it in this week that would be fantastic.

@lgandecki I'm on it, I'll do some testing today and tomorrow. Thanks

great! thanks as well :)

This can be marked as fixed by #538

Was this page helpful?
0 / 5 - 0 ratings

Related issues

shyamprasadsdet picture shyamprasadsdet  ·  5Comments

EmmanuelDemey picture EmmanuelDemey  ·  5Comments

hiaux0 picture hiaux0  ·  6Comments

PrinceJohn picture PrinceJohn  ·  3Comments

leegee picture leegee  ·  4Comments