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

spec-reporter prints retries #5099

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

CheadleCheadle
Copy link

@CheadleCheadle CheadleCheadle commented Feb 22, 2024

PR Checklist

Overview

I've implemented a function called logRetries() which displays the number of the current retry along with the total number of retries, provided that the total number of retries is greater than zero. This output is formatted below the test title and highlighted in yellow as requested for ecosystem consistency. I would greatly appreciate any feedback on this addition!
Example:
retries

@coveralls
Copy link

Coverage Status

coverage: 94.346% (+0.01%) from 94.335%
when pulling b5d4d68 on CheadleCheadle:spec-reporter-retries
into b88978d on mochajs:master.

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello again! 😄

Left some thoughts - I'm excited to see this come in!

lib/reporters/spec.js Outdated Show resolved Hide resolved
lib/reporters/spec.js Outdated Show resolved Hide resolved
bin/mocha.js Outdated Show resolved Hide resolved
lib/reporters/spec.js Show resolved Hide resolved
@JoshuaKGoldberg JoshuaKGoldberg added the status: waiting for author waiting on response from OP - more information needed label Mar 4, 2024
Copy link
Author

@CheadleCheadle CheadleCheadle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored to include retry on same line(retry x1).

@JoshuaKGoldberg JoshuaKGoldberg added status: in triage a maintainer should (re-)triage (review) this issue semver-minor implementation requires increase of "minor" version number; "features" semver-major implementation requires increase of "major" version number; "breaking changes" and removed status: waiting for author waiting on response from OP - more information needed semver-minor implementation requires increase of "minor" version number; "features" labels Jul 2, 2024
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't sure if you're ready to re-request review, but just in case 🙂

@JoshuaKGoldberg JoshuaKGoldberg added status: waiting for author waiting on response from OP - more information needed and removed status: in triage a maintainer should (re-)triage (review) this issue labels Jul 3, 2024
Copy link
Member

@voxpelli voxpelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some feedback on the implementation.

On top of that: The bin/mocha.js has an unrelated permission change.

Comment on lines 91 to 98
} else {
fmt =
indent() +
color('checkmark', ' ' + Base.symbols.ok) +
color('pass', ' %s') +
color(test.speed, ' (%dms)');
Base.consoleLog(fmt, test.title, test.duration);
Base.consoleLog(fmt, test.title, test.duration, logRetries());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would opt for something like this instead, to keep in the style of the others:

  runner.on(EVENT_TEST_PASS, function (test) {
    var fmt =
      indent() +
      color('checkmark', '  ' + Base.symbols.ok) +
      color('pass', ' %s');
    var fmtVars = [test.title];

    if (test.speed !== 'fast') {
      fmt += color(test.speed, ' (%dms)');
      fmtVars.push(test.duration);
    }

    if (test._currentRetry > 0) {
      fmt += color('bright yellow', ' (retry x%d)');
      fmtVars.push(test._currentRetry);
    }

    Base.consoleLog.apply(Base, [fmt].concat(fmtVars));
  });

Included message within format so it can be passed into `Base.consoleLog` as one argument
*/
var retryFmt = color(
'bright yellow',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To really stick with the theme I think we should add a 'retries' color here:

/**
* Default color map.
*/
exports.colors = {
pass: 90,
fail: 31,
'bright pass': 92,
'bright fail': 91,
'bright yellow': 93,
pending: 36,
suite: 0,
'error title': 0,
'error message': 31,
'error stack': 90,
checkmark: 32,
fast: 90,
medium: 33,
slow: 31,
green: 32,
light: 90,
'diff gutter': 90,
'diff added': 32,
'diff removed': 31,
'diff added inline': '30;42',
'diff removed inline': '30;41'
};

And possibly stick even more with the style of eg. durations and resolve to the color in base.js:

mocha/lib/reporters/base.js

Lines 369 to 377 in 24560c1

runner.on(EVENT_TEST_PASS, function (test) {
if (test.duration > test.slow()) {
test.speed = 'slow';
} else if (test.duration > test.slow() / 2) {
test.speed = 'medium';
} else {
test.speed = 'fast';
}
});

And maybe do a similar style and have two levels: 'some retries' / 'many retries' With only the latter resolving to bright yellow

@@ -67,20 +67,34 @@ function Spec(runner, options) {
});

runner.on(EVENT_TEST_PASS, function (test) {
function logRetries() {
if (test._currentRetry > 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The underscore in _currentRetry is an old-school practice for indicating that its a private property, so we shouldn't rely on it here I think as its not intended for outside consumption.

If we want it to be exposed we should make it an explicit public property, like duration, and probably name it retries or retryCount or such

We should use the public .currentRetry() instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major implementation requires increase of "major" version number; "breaking changes" status: waiting for author waiting on response from OP - more information needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should the spec reporter report retries?
4 participants