Skip to content

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

@rvagg rvagg force-pushed the duration_ms_note branch from a9f75c5 to 92662e3 Compare June 8, 2016 10:58
@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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
rvagg added a commit that referenced this pull request Jun 13, 2016
PR-URL: #7216
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
@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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
evanlucas pushed a commit that referenced this pull request Jun 16, 2016
PR-URL: #7216
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
@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