enhancement: granular control over retries

[dalimian]

when a test is being retried the beforeEach hook runs as expected, but the before hook does not

[ScottFreeCode]

This behavior is correct/consistent: before only runs once per suite (and the retries functionality only reruns individual tests and not whole suites). Is there a particular problem you are trying to solve by running certain code both at the start of the suite and before each retry?

[dalimian]

my case is that I have a before hook that opens a url followed by bunch of describe blocks that test different sections of that page. most of the flakiness is typically with the page itself (loading the page on test env), so if a specific test failed it makes no sense for me to retry that test without reloading the page. Sure I could change the before to a beforeEach but that slows my test unnecesserily (by reloading the page for testing each section).

[dalimian]

@ScottFreeCode

[ScottFreeCode]

Hi @dalimian, sorry about the wait -- I haven't had a whole lot of time to think this one over.

I can tell you that that's a fairly unusual case in the grand scheme of things -- we are generally cautious about changing anything risky (e.g. potentially confusing or potentially breaking code relying on the old behavior), but doubly so for unusual cases. On the other hand, we want to help people resolve all the cases we can.

Off the top of my head, some kind of "onRetry" hook might be the most appropriate way to handle this kind of situation, but somebody would have to figure out how to implement it and we'd need to think a little about how much potential there is for abuse.

A clumsy alternative might be to wrap tests a retry-like function instead of using Mocha's retry:

function retryable(test) {
  return function(done) { // remove all `done` if tests should not have `done`
    for (var index = 0; index < retryCount; index += 1) {
      try {
        return test(done)
      } catch (ignore) {}
      refreshPage()
    }
    return test(done)
  }
}

it("example test", retryable(function(done) {
  ...
})

[dalimian]

thanks @ScottFreeCode, we ended up doing something like this

  before(() => {
      loadPage();
    }
  });
  beforeEach(() => {
    if (this.ctx.currentTest._currentRetry) {
      loadPage();
    }
  });

this gives us best of both worlds - speedy execution if everything passes, and the behavior we desired for failing tests

don't really like referencing props with underscores - _currentRetry but other than that pretty clean no?

[ScottFreeCode]

don't really like referencing props with underscores - _currentRetry but other than that pretty clean no?

Indeed!

I've chewed this over and I can think of a few approaches:

• Make _currentRetry an officially public property.
  • Basically canonicalizes (canonizes?) your solution.
  • Trivial to implement.
  • I have a vague recollection that it's been discussed as a potential solution to other use cases, but can't recall whether there were any concerns (e.g. that some uses might be antipatterns); searching the issues list for it might bring up something interesting, when I have time for it.
• Make an onRetry hook.
  • Usage-wise, pretty much the same except no if wrapper necessary.
  • Should be straightforward to implement, but not necessarily trivial.
  • Potentially problematic in the ways the hooks tend to be problematic -- e.g. it's one more thing where we have to ask how it interacts with nested suites.
• Add a setting for whether before and after should be run on retries.
  • Superior usage: avoids having to call setup (and/or cleanup) code in two places.
  • Ideally, should be able to be set for each individual hook, in case only certain things need to be done over on retry. (If it's like most Mocha settings, it would also be possible to set a default at a suite level or even a global level via the outermost Mocha APIs such as the CLI.)
  • However, I'm not sure how that would be set for after hooks before they run, so it might only be possible to set it one way or the other for all after hooks in a suite.
• Change nothing and make note of _currentRetry as a workaround.
  • Avoids officially endorsing the practice of doing things when a test is retried. (This may or may not be desirable -- as previously alluded to, I don't remember whether there have previously been concerns about this. If there haven't, then obviously this kind of caution isn't necessary.)
  • Still works as long as _currentRetry is in the implementation (which it probably will be since the number of retries has to be tracked somewhere).
  • Arguably not great inasmuch as it conflates "not officially encouraged" with "implementation detail that could be subject to change".

Not really sure where I lean on this one, as there are a lot of tradeoffs here (whether we want to officially support this case, whether we want to go for a solution that's the most weird/boilerplatey in usage but easiest to implement/maintain, whether we could find a way to tag some after hooks as applying on retry and others as not...)

[dalimian]

Thanks alot!

tough choices

my favorite is prob Add a setting for whether before and after should be run on retries. - it's simplest both as an an implementation and concept to grasp. it would also help people discover this feature easy. it's not as flexible as others tho (like with onretry hook u can check which test is failing etc).

downsides of Change nothing and make note of _currentRetry as a workaround. or Make _currentRetry an officially public property is feature discoverability. Also, first reaction of people seems to be "this is a bit weird"

[bwpetersen]

I'm working on the same tests as @dalimian. I think one way to solve the problem is to have different types of retries: describe level and it level.

To clarify what I mean:

it retry

When an it function fails, the it is rerun. Rerunning the it causes beforeEach functions to be called. This is what I believe mocha does currently more or less.

describe retry

The describe runs through all of its tests. If there are any it function failures, the describe is rerun. Rerunning the describe causes before to be called once and every failing it to be rerun. Nested describe functions are only rerun if one of its child it functions fail. This is what we want our retries to do.

An undesired consequence of our current solution of the conditional beforeEach, is that if there are multiple it failures then we have to reload the page for each it failure, rather than for each retry. The onRetry hook would have the same issue.

Add a setting for whether before and after should be run on retries.

This sounds better than the alternatives provided.

[ScottFreeCode]

Hmm, the option to retry all failing tests in a describe in one go, with before and after, is very interesting. I'll have to think about whether that covers all the same use cases, or whether there's a way to combine that with any of the other ideas to cover all use cases.

[boneskull]

I'm not really convinced this casts a wide enough net to be in core... feel free to make the case though. I see this as being specifically useful for certain integration or functional tests, but it might be too narrow.

[jimlindeman]

Hmm, the option to retry all failing tests in a describe in one go, with before and after, is very interesting. I'll have to think about whether that covers all the same use cases, or whether there's a way to combine that with any of the other ideas to cover all use cases.

This is a very useful case for managing debug-level logging in automated testing, the example being case where if we turn on our debug logging for all tests, we overflow our allowed log-length in Travis. If we had this ability, we could run our tests at log-level INFO, but have it re-run the failing tests at log-level DEBUG, enabling us to gather the root-cause information that's normally only exposed in debug-level logs.

[Art-auto]

I'm working on the same tests as @dalimian. I think one way to solve the problem is to have different types of retries: describe level and it level.

To clarify what I mean:

it retry

When an it function fails, the it is rerun. Rerunning the it causes beforeEach functions to be called. This is what I believe mocha does currently more or less.

describe retry

The describe runs through all of its tests. If there are any it function failures, the describe is rerun. Rerunning the describe causes before to be called once and every failing it to be rerun. Nested describe functions are only rerun if one of its child it functions fail. This is what we want our retries to do.

An undesired consequence of our current solution of the conditional beforeEach, is that if there are multiple it failures then we have to reload the page for each it failure, rather than for each retry. The onRetry hook would have the same issue.

Add a setting for whether before and after should be run on retries.

This sounds better than the alternatives provided.

This is a highly useful feature on our app, is there a chance it will be implemented at any time?

[JoshuaKGoldberg]

Per #5027, we're not looking to significantly overhaul Mocha any time soon. Given that you can implement this in userland with wrapper functions and that less than one end user has interacted with this on average per year in the >6 years it's been open, closing as out of scope. Cheers all!

Please yell at me if you strongly believe this feature must exist in Mocha core. 🙂