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

TAP output for citgm-all #85

Closed
gibfahn opened this issue Feb 25, 2016 · 9 comments
Closed

TAP output for citgm-all #85

gibfahn opened this issue Feb 25, 2016 · 9 comments

Comments

@gibfahn
Copy link
Member

gibfahn commented Feb 25, 2016

I'm currently using citgm-all with a lookup.json file as part of Jenkins CI, and it'd be useful to have citgm-all output a TAP file in the same way node tools/test.py does with its --progress=tap option. That way, with the Jenkins TAP plugin it'd be really easy to redirect the citgm-all output to a .tap file for a range of tests ( avoiding having to dig through the console log on every failure).

The individual npm modules often have their own TAP outputs for their tests, but an overall TAP file with one TAP result for each module would be useful.

I made a draft of what I imagined the tap output would look like, let me know if it seems feasible. I'm not sure how much work it would take to add this.

1..2
ok 1 express
  ---
  duration_ms: 100.137
  ...
not ok 2 lodash
   # not ok 20 lodash-test-connection
   # ECONNRESET
   # 
  ---
  duration_ms: 60.88  
  ...

Feedback/criticism welcome!

@MylesBorins
Copy link
Member

I'd love to see tap results, and this has been in the back of my mind for a while.

The only issue with the tap reporting you are suggesting is that currently there is no easy way to get the granular information about "why" a particular module is failing. All of the testing data comes out of the child_process running npm, and since we don't know 100% what reporter the modules is using, trying to translate all extensive testing information seems a bit like boiling the ocean to me.

That being said... having some cleaner tap based output for CI would make looking at the results SO much nicer.

Currently the state of all the modules is stored in objects throughout the build, and then passed the a reporter which prints the final results. There is both a normal and a markdown reporter, with a cli flag to select it. Adding an extra reporter which can generate tap output should not be a super difficult task!

@MylesBorins
Copy link
Member

@gibm the above PR introduces a tap adapter, hoping to have this out soon

This is what the output will look like in travis --> https://ci.nodejs.org/job/thealphanerd-tap-smoker/6/

@gibfahn
Copy link
Member Author

gibfahn commented Feb 26, 2016

Wow, that was quick work @thealphanerd ! The CI already looks a lot nicer.

As for the information about why a module failed (i.e. which tests failed), presumably the problem is that there's no standard way to report success or failure for individual tests in a module. As far as I can see the standard ways are either TAP again, or to use ticks or crosses.

verbose: npm-test:           | ok test-isUrl.js .................. 17/17
verbose: npm-test:           | ok test-json-request.js ........... 28/28
verbose: npm-test:           | not ok test-localAddress.js ......... 5/5

or

verbose: npm-test:           | sync                
verbose: npm-test:           | ✔ test_salt_length  
verbose: npm-test:           | ✔ test_salt_no_params
verbose: npm-test:           | ✔ test_salt_rounds_is_string_number
verbose: npm-test:           | ✔ test_salt_rounds_is_NaN

Would it be possible to put any lines that began with not ok or a ✘ in the comment section of a failed test?

That way when you open up a failure in Jenkins it would include those lines. Obviously this wouldn't work for modules which used other reporting methods, but it would definitely be a start. I'm not sure how difficult this would be to implement though.

@gibfahn
Copy link
Member Author

gibfahn commented Feb 26, 2016

Looks like you can also get NPM errors:

verbose:                     | > echo "Error: no test specified" && exit 1       
verbose: npm-test:           | Error: no test specified
verbose: npm:                | npm ERR! Test failed.  See above for more details.
error:   failure             | The canary is dead. 

@MylesBorins
Copy link
Member

@gibm for the moment the difficult part would be the way I am generating the tap currently.

Right now there are three post-mortem reporters.. the logger / markdown / tap

All of them work using the same array of objects with some pretty basic data. We would have to hook into the child process output of each run to store the test data, and then provide that to the object.

I would be concerned that if we attmped to do it the way I am currently doing it we would have a problem, as we would hit max string size storing things in data.

Likely we would have to upgrade all the reporting to be on some sort of stream based system. tbqh I think it may be best to get the basic tap reporting in... and then attempt a refactor to bring forward more useful data.

For simplicity right now if you want to see the extended output of a single module you can search for "the test for failed"

That is not really desirable though.

I'm going to think on this, please feel free to fork my current tap proposal and attempt to implement something. All ideas are shallow with enough eyes 😄

@MylesBorins
Copy link
Member

So I did a really naive / nasty approach to getting this data. Relying on the current pattern of storing everything in memoery... hopefully we don't hit max string size and explode

--> https://ci.nodejs.org/job/thealphanerd-tap-smoker/8/

We store the output of the child process task and append that to the module object in the context. When we hit the reporter all this data is in there now waiting to be parsed / sanitized.

The current approach will only print the test suite data for failures at the end reporting for the logger and the markdown output. It will alternatively print all test data when reporting tap... so we should be able to dig in deeper to the results

@gibfahn
Copy link
Member Author

gibfahn commented Feb 29, 2016

@thealphanerd I'm still learning about Node and CitGM, but I'll definitely fork your changes and have a play. I think what you've done is definitely a great start, should be very helpful for Jenkins debugging, it would be good to get your changes committed so we can iterate on them.

@MylesBorins
Copy link
Member

@gibm --> #86 already committed and awaiting review :D

@MylesBorins
Copy link
Member

this has landed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants