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

test: add note about duration_ms in TAP reporter #7216

Closed
wants to merge 2 commits into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Jun 8, 2016

Following on from #7214 (including that commit), adds an inline comment for future reference

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jun 8, 2016
@mscdex
Copy link
Contributor

mscdex commented Jun 8, 2016

@rvagg I think you accidentally included some unrelated (http) changes in your commit.

@mscdex mscdex added test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. and removed http Issues or PRs related to the http subsystem. labels Jun 8, 2016
@rvagg
Copy link
Member Author

rvagg commented Jun 8, 2016

doh! that was from testing #7211, thanks for picking that up @mscdex

@gibfahn
Copy link
Member

gibfahn commented Jun 8, 2016

@rvagg Thanks for picking up on this! I couldn't find any documentation about it anywhere (e.g. on the TAP website) but I can see that the Jenkins TAP parser understands duration_ms so I guess there's no alternative.

@rvagg
Copy link
Member Author

rvagg commented Jun 8, 2016

I really should have documented this earlier @gibm, I went though this exact pain very early on because it frustrated me too. I ended up in the Jenkins source code just to verify that it relied on it in this form. I think it's just one of those standards that have appeared and been widely enough adopted to be stuck in a semi-official state.

@cjihrig
Copy link
Contributor

cjihrig commented Jun 8, 2016

LGTM

@jasnell
Copy link
Member

jasnell commented Jun 8, 2016

LGTM
(and thanks @rvagg ... my apologies when I reviewed the original change I had completely forgotten that it was a tap thing...)

@targos
Copy link
Member

targos commented Jun 8, 2016

LGTM

@jbergstroem
Copy link
Member

LOOKS GOOD TO ME

@gibfahn
Copy link
Member

gibfahn commented Jun 8, 2016

@rvagg Looks like there's a PR that will go into a future version of the TAP plugin which requires duration_ms output to be in milliseconds (node's is currently in seconds).

jenkinsci/tap-plugin#6

Not sure how we'd deal with this.

EDIT: It might be worth adding a duration_s option to the Jenkins tap plugin, but you'd still have a breaking change.

@rvagg
Copy link
Member Author

rvagg commented Jun 8, 2016

oh my, that's terrible, I've added a comment over there

@rvagg rvagg closed this Jun 13, 2016
@rvagg rvagg deleted the duration_ms_note branch June 13, 2016 11:55
rvagg added a commit that referenced this pull request Jun 13, 2016
This reverts commit d413378.

PR-URL: #7216
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
rvagg added a commit that referenced this pull request Jun 13, 2016
PR-URL: #7216
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
@rvagg
Copy link
Member Author

rvagg commented Jun 13, 2016

1d4c799 & 05de4d7

evanlucas pushed a commit that referenced this pull request Jun 16, 2016
This reverts commit d413378.

PR-URL: #7216
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
evanlucas pushed a commit that referenced this pull request Jun 16, 2016
PR-URL: #7216
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
@evanlucas evanlucas mentioned this pull request Jun 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants