mark Runnable#retries and Runnable#currentRetry methods as public

[jan-molak]

This change makes it easier for Mocha reporters to report retriable and regular tests differently, should they choose to do so.

Description of the Change

I'd like the Serenity/JS Mocha reporter to be able to report differently on regular and retriable Mocha scenarios. In order to do so, a Mocha reporter needs to be able to tell the difference between the two.

The easiest way to do it is to mark the already existing Runnable#retries and Runnable#currentRetry methods as public to expose the required information.

Alternate Designs

Any alternative designs would require marking at least Runnable#currentRetry as public, so changing the JSDoc seems like the most efficient way to reach my objective.

Why should this be in core?

This functionality can't be implemented in an external module.

Benefits

This change allows any Mocha reporter to treat retriable and regular scenarios differently and therefore provide better quality information to developers using them. If the PR makes it to core, I'm also planning to raise another one for @types/mocha

Possible Drawbacks

Marking those APIs as public might encourage developers to use them in their tests, which they probably shouldn't be doing. Having said that, they can do that already.

Applicable issues

N/A

[jsf-clabot]

All committers have signed the CLA.

[coveralls]

Coverage remained the same at 93.932% when pulling 904609d on jan-molak:features/public-retry-apis into 7c8896c on mochajs:master.

[boneskull]

Why do you need Runnable#retries() and not Context#retries()? Runnable is essentially "abstract"...

The interface in use (e.g., bdd) will call Context#retries(), which should be fair game, and is probably erroneously marked @private.

How are you accessing a Runnable instance (or sub-pseudoclass-instance)?

[jan-molak]

Hey @boneskull,

I need both retries and currentRetry, so Runnable seemed (?) like the place to do it.

Here's an example of how I use those APIs.

So in my reporter:

runner.on(Runner.constants.EVENT_TEST_RETRY,  (test: Test, err: Error) => {
    if (test.retries() >= 0) {
      // do something with test.currentRetry();
    }                
});

I could move the change to interface visibility from Runnable#retries() to Context#retries() (or apply it in both places?)

However, I don't see currentRetries on Context?

Let me know which approach you prefer and I can update the PR.

Thanks,
Jan

[boneskull]

OK, so that looks like a reporter. That example code should fail in parallel mode, since test.retries() is not a function.

This is what you should do:

1. Add $$retries: this._retries to the object returned by Test#serialize(). This convention will tell the object serializer to create a function retries() which returns the value of this._retries. Runnable#retries() is just a setter/getter.
2. All functions in that object (denoted by a $$ prefix) should be public, since they are intended to be consumed by reporters. Mark the following functions in Runnable as @public (you may need @memberof as well; if you want to generate the API docs, run npm start docs.api and open docs/_site/api/index.html):
   1. slow()
   2. isPending()
   3. retries()
   4. currentRetry()
3. slow, isPending and currentRetry should have been @public; this is in error. However, given slow() and retries() are both combination setters/getters and reporters should not use them as setters, I think it's wise to make this clear in the documentation. I don't know how to do that other than remove the @param tags from each of these, so they appear to not accept parameters.

It's pretty obvious the combination setters/getters are problematic from an API surface standpoint. We really ought to reconsider all of them, because of problems like this and the lack of uniformity.

[JoshuaKGoldberg]

Closing as an old and stale draft. Someone yell at me if this should be reopened. 🙂