Nesting hooks within other hooks causes the test to never finish (runs forever)

[todd-m-kemp]

Current behavior:

Since upgrading to Cypress 5.0.0, I cannot get any tests to pass. Tests will run through all the code, but when they get to the end, the test does not get marked as passed and the timer keeps counting up.

Desired behavior:

Tests end and move on the next test/spec.

Test code to reproduce

context('Never-ending Test', function () {
  it('Test', function () {
    cy.visit('http://wikipedia.com')
    cy.get('#searchInput')
      .type('Cypress.io')
    cy.get('.pure-button')
      .click()
    // Test never ends.
  })

  it('Next Test', function () {
    // Test never starts.
    cy.visit('http://wikipedia.com')
  })
})

Versions

Cypress 5.0.0
macOS: 10.14.6
Browser: Chrome 84.0.4147.125, Firefox 79, Electron 83

[kuceb]

@todd-m-kemp if you open the F12 console and enter Cypress.version, do you get 5.0.0? It's possible cypress_runner.js is being cached somehow.

[todd-m-kemp]

Hi @bkucera ! Thanks for the quick response, but unfortunately, it does report the Cypress version as 5.0.0.

[kuceb]

@todd-m-kemp whoops on second thought that won't rule out a caching issue. Could you try Cypress.getTestRetries?

[todd-m-kemp]

@bkucera: As requested:

Is it possible there is something related to code migration I missed? I updated blacklistHosts to be blockHosts but all the other notes in the Migration Guide seemed irrelevant to my codebase.

[kuceb]

I'm not able to reproduce, have you made any changes to support files / plugin files? If you run the spec code in a blank project, are you still able to repro?

also contents on cypress.json would be helpful

[todd-m-kemp]

I think I have found the culprit. I have the code

beforeEach(function () {
  const maxLength = 2; // (minutes)

  const timeoutId = setTimeout(() => {
    throw new Error(`This test failed because it ran longer than the maximum permitted time for a single test (${maxLength} minutes). `);
  }, 60000 * maxLength);

  afterEach(() => {
    clearTimeout(timeoutId);
  });
});

in my /support/index.js file because we wanted any test that took longer than 2 minutes to fail out. If I take this out, both tests pass in 7 seconds. With this code left intact the first test will run continually and never pass or fail, even if it goes beyond the 2 minute mark.

This had been working just fine in version 4.12.0 and stopped when I upgraded to version 5.0.0.

[kuceb]

I see, it may be a problem with the nested hook. Maybe related to #8214

You're registering an afterEach inside the beforeEach hook (registering as many afterEach hooks as you have tests), whereas I think you should be writing:

let timeoutId
beforeEach(function () {
  const maxLength = 2; // (minutes)

  timeoutId = setTimeout(() => {
    throw new Error(`This test failed because it ran longer than the maximum permitted time for a single test (${maxLength} minutes). `);
  }, 60000 * maxLength);
});

afterEach(() => {
  clearTimeout(timeoutId);
});

[todd-m-kemp]

It could be related to #8214 but I'm not certain because that seems to be a regression going from 4.9.0 to 4.10.0 and I had been using the nested afterEach as I described initially without difficulty in version 4.12.0. 🤷‍♂️

As for why I used the afterEach nested the way I did, I actually got this code snippet from here. I had initially tried to rewrite it in the manner you have above, but what I found was that when any one test went above the time limit, Cypress failed the desired test but then did not start any other tests from the same spec; it just left them untested. When the code is used as I've written it, a test that goes over the time limit gets failed but other tests in the spec will still start running and pass (provided they don't also exceed the time limit).

Thanks for your input. 😄 Is there any more information you would like from me on this matter right now? I think I have given you everything you should need to be able to reproduce at this point.

[bahmutov]

@todd-m-kemp maybe use this recipe to stop long-running tests https://github.com/cypress-io/cypress-example-recipes/tree/master/examples/fundamentals__timeout

[todd-m-kemp]

Thanks @bahmutov ; this doesn't seem to be quite what I'm looking for as it seems to only be capable of doing timeouts on a per test basis, but I still appreciate it. A method that could be applied globally to all tests (like the one I had been using up until now) would be preferable, but maybe I can get away with sprinkling this in various places throughout the codebase as needed. I would still like to be able to use the code I had been using before as it seems to be the best option right now.

[jennifer-shehane]

While this is a separate bug regarding nested hooks (since this only appears in 5.0.0), the fix for this will likely be the same as for #8214, we will error when nesting hooks.

I'd be open to hearing any legitimate arguments for needing nested hooks, but our team can't really think of any.

Repro

beforeEach(() => {
  afterEach(() => {})
})

it('test', () => {
  expect(true).to.be.true
})

4.12.1

5.0.0

Workaround

Do not nest hooks within each other - nesting hooks will likely not be supported behavior moving forward, so ensure your code does not have any nested hooks.

beforeEach(() => {})

afterEach(() => {})

it('test', () => {
  expect(true).to.be.true
})

[todd-m-kemp]

Hi @jennifer-shehane ! I'm glad to hear that the source of the problem has been identified.

As for your request as to why one would want nested hooks, I'll try to explain why I am using one and hopefully that will provide some insight into why it is something I need.

The majority of the Cypress tests for the application that I work with involve waiting for an encoding process on a file upload to complete before proceeding with the remainder of the test. To wait on that encoding process to complete, we poll the backend in order to wait for the status of that encoding process to switch from ongoing to complete. Only when the encoding is complete, do we proceed with the rest of the test. Because of the potential for the encoding process to become "stuck" (i.e. never switching from ongoing to complete) we wanted to add a stop-gap to make sure that any test running longer than a preset amount of time would fail, and then proceed to the next test. To make this happen, we use the following code in the index.js file, which does have a nested hook.

beforeEach(function () {
  const maxLength = 2; // (minutes)

  const timeoutId = setTimeout(() => {
    throw new Error(`This test failed because it ran longer than the maximum permitted time for a single test (${maxLength} minutes). `);
  }, 60000 * maxLength);

  afterEach(() => {
    clearTimeout(timeoutId);
  });
});

With this code in place, any test in the entire suite that runs longer than 2 minutes will fail, but more importantly _ each test in the full suite still runs_.

If you rewrite the code so that it is no longer nested like @bkucera has suggested above

let timeoutId
beforeEach(function () {
  const maxLength = 2; // (minutes)

  timeoutId = setTimeout(() => {
    throw new Error(`This test failed because it ran longer than the maximum permitted time for a single test (${maxLength} minutes). `);
  }, 60000 * maxLength);
});

afterEach(() => {
  clearTimeout(timeoutId);
});

the behaviour is similar, but there is an important difference. When a single test takes longer than 2 minutes to run, that test is failed, but all subsequent tests in the same spec do not get run. This is why I find the suggested un-nested approach unsuitable for my needs. I require all tests to be run: if one takes too long, then it should fail, but the tests after should still be run.

I looked into using the recipes provided by @bahmutov but unfortunately that approach seems to only allow you to set a timeout on a per-test basis and I require something global so I don't have to apply a timeout to every single test. If a workaround that allows me to achieve a globally set test timeout that doesn't cause other tests to not run when a single test does time out that doesn't use a nested hook could be provided that would be amazing but as it stands now, I think that this provides a pretty good argument for why a nested hook is required.

Please let me know if you have any questions, and thanks again for your help with this. 😃

[kuceb]

@todd-m-kemp we skip the remaining tests in a suite when a test fails in a hook. So if your test "times out" while it's in a beforeEach hook for example, the remaining tests in the suite are skipped no matter how it's written. So it may be that you just happen to timeout in the test body when trying out the nested hook code. It shouldn't make any difference in reality.

If you play with the timeout value you may notice different results based on where your test fails

[todd-m-kemp]

@bkucera : Thank you so much for that explanation; I've tested it out and it does appear that is exactly what is happening. 😄 I'll have to rework that section of code to remove the nested hook if the result here is that nested hooks soon won't be supported.

[bahmutov]

@todd-m-kemp I thought how to extend our recipe to allow setting global test timeout, here is the solution cypress-io/cypress-example-recipes#549

[todd-m-kemp]

@bahmutov : Thank you very much for extending the recipe. I've given it a shot but I did run into a problem with it which I've logged here.

[cypress-bot]

The code for this is done in cypress-io/cypress#8379, but has yet to be released.
We'll update this issue and reference the changelog when it's released.

[cypress-bot]

Released in 5.1.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v5.1.0, please open a new issue.