Conversation
|
Review requested:
|
nodejs-github-bot
added
lib / src
JakobJingleheimer
left a comment
JakobJingleheimer
left a comment
There was a problem hiding this comment.
Looking good so far. I expected to see a loop somewhere; did you merely not get to that yet (since this is a draft) or did I miss some magic somewhere?
PS Sorry this took me a while to get to.
| @@ -87,6 +87,9 @@ function formatTestReport(type, data, showErrorDetails = true, prefix = '', inde | |||
| } | |||
| } else if (expectFailure !== undefined) { | |||
| title += ` # EXPECTED FAILURE`; | |||
| } else if (flaky !== undefined && flaky > 0) { | |||
| const retryText = flaky === 1 ? 're-try' : 're-tries'; | |||
| title += ` # FLAKY ${flaky} ${retryText}`; | |||
lib/internal/test_runner/test.js
Outdated
| let { fn, name, parent } = options; | ||
| const { concurrency, entryFile, expectFailure, loc, only, timeout, todo, skip, signal, plan } = options; | ||
|
|
||
| const { concurrency, entryFile, expectFailure, flaky, loc, only, timeout, todo, skip, signal, plan } = options; |
There was a problem hiding this comment.
nit: this is getting really long
| const { concurrency, entryFile, expectFailure, flaky, loc, only, timeout, todo, skip, signal, plan } = options; | |
| const { | |
| entryFile, | |
| expectFailure, | |
| flaky, | |
| loc, | |
| only, | |
| plan, | |
| signal, | |
| skip, | |
| timeout, | |
| todo, | |
| concurrency, | |
| } = options; |
package-lock.json
Outdated
There was a problem hiding this comment.
I think this should not be committed?
| @@ -78,6 +78,10 @@ function reportTest(nesting, testNumber, status, name, skip, todo, expectFailure | |||
| line += ` # TODO${typeof todo === 'string' && todo.length ? ` ${tapEscape(todo)}` : ''}`; | |||
| } else if (expectFailure !== undefined) { | |||
| line += ' # EXPECTED FAILURE'; | |||
| //should we use flaky >=0 here? for always printing 0 retries | |||
| } else if (flaky !== undefined && flaky > 0) { | |||
| const retryText = flaky === 1 ? 're-try' : 're-tries'; | |||
| line += ` # FLAKY ${flaky} ${retryText}`; | |||
There was a problem hiding this comment.
In the feature proposal, the output cites the number of re-tries it took to succeed. So I think the name for flaky should be changed to something like flakyRetriedCount to disambiguate it with the maximum number of re-tries to attempt.
# FLAKY 42 re-tries
|
Hi @vespa7, are you still planning to continue this PR? If not, I’d like to pick this up and open a follow-up PR based on the same idea. If that works for you, I’ll make sure to credit your original work. |
Yes I will publish the pr tomorrow |
JakobJingleheimer
left a comment
JakobJingleheimer
left a comment
There was a problem hiding this comment.
Thanks for this!
I agree the formatting changes are improvements, but they're not germane to this PR, and they make it rather difficult to see the real changes, so please undo them 😰
| * `testNumber` {number} The ordinal number of the test. | ||
| * `todo` {string|boolean|undefined} Present if [`context.todo`][] is called | ||
| * `skip` {string|boolean|undefined} Present if [`context.skip`][] is called | ||
| * `flakyRetriedCount` {number|undefined} The number of retries taken for a |
There was a problem hiding this comment.
This feels a little wordy to me. Maybe just retryCount?
4 participants
nodejs/test-runner - Feature proposal:
flaky