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

Add test-tap-file npm script that outputs results to a separate file. #2359

Closed
wants to merge 1 commit into from

Conversation

misterdjules
Copy link
Contributor

This allows continuous integration platforms to retrieve and parse tests
results easily.

This allows continuous integration platforms to retrieve and parse tests
results easily.
@misterdjules
Copy link
Contributor Author

This PR is a discussion starter.

We have a job setup on Node.js' Jenkins platform that tests several large and popular Node applications against Node's v0.12 branch to make sure that nothing obvious is broken prior release.

In order for Jenkins to be able to present tests results in a meaningful way to us so that we can investigate potential tests failures, we need to retrieve and parse tests results. Currently, express' npm test scripts output tests results to standard err and out and use an output format that is not easily parsed by a computer.

What we'd like to have is:

  • Tests results outputted in a format that can be parsed by a computer (like TAP).
  • Tests results outputted in a place where they can be retrieved easily by the continuous integration platform that runs the tests.

We could run the mocha command line manually, or we could change the package.json file on the fly before running the tests, but ideally we'd like to be able to run a command such as npm run test-tap-file and know that the tests are run correctly in the future without having to maintain our own changes to express.

The changes in this PR are not ideal because:

  • The test-tap-file npm script duplicates the test script and only changes the reporter. We could add a script that runs the tests and that takes a reporter as an argument to avoid this duplication. The script would need to work on all platforms supported by Node, and would thus probably be implemented in JavaScript.
  • It uses the tap-file module, which doesn't seem to be a mature module (e.g, it writes synchronously to the output file, it is not used by a lot of developers and it's still at version 0.0.2).

Please let me know what you think.

@rlidwka
Copy link
Member

rlidwka commented Sep 16, 2014

Use mocha --reporter tap and parse its stdout.

There is no need to bring tap-file here.

@rlidwka
Copy link
Member

rlidwka commented Sep 16, 2014

As for duplication, maybe add a makefile that runs stuff which you can configure using env variables, i.e.:

REPORTER ?= spec
test:
    mocha --require test/support/env --reporter ${REPORTER} --bail --check-leaks test/ test/acceptance/

@misterdjules
Copy link
Contributor Author

@rlidwka Thank you for looking into this and for your suggestions.

The problem with using a Makefile is that make is not easily available on platforms like Windows. Since I'd like to be able to test express on Windows without having to setup and manage make manually, it would be easier to implement it with a Node.js script.

@rlidwka
Copy link
Member

rlidwka commented Sep 16, 2014

The problem with using a Makefile is that make is not easily available on platforms like Windows.

We're talking about CI integration, right? Those things don't run on windows.

@dougwilson
Copy link
Contributor

I don't have a problem adding a test-tap script so you can do

npm run test-tap > /path/to/output.tap

Do you only test master or do you also test the supported and active 3.x branch?

Those things don't run on windows.

Um, yes they do. They even have a Travis CI that is Windows available for free like Travis.

@dougwilson
Copy link
Contributor

@misterdjules let me know if you need this on the 3.x branch; that branch is actually used even more by people than the master branch, so you should probably be testing that to, especially since those tests are drastically different.

dougwilson added a commit that referenced this pull request Sep 16, 2014
@dougwilson
Copy link
Contributor

I have implemented this. You can make your test command to be npm run test-tap and pipe STDOUT to whatever file you want if you need files and your testing environment cannot just read from STDOUT directly. The change landed on the 3.x branch because you have to test the 3.x branch if you actually want to know if you are breaking the most popular express. Eventually the changes will merge up into master.

@misterdjules
Copy link
Contributor Author

@dougwilson Thank you very much for your help, I really appreciate it. Also thank you for the advice on the 3.x branch. I'm going to test all this on Jenkins and I'll let you know how it goes.

@dougwilson
Copy link
Contributor

No problem. Also, I forgot to mention, the timeline for the 3.x into master merge with your testing changes is sometime tonight with a express 4.9.1 release (it's morning for me, so that means probably in about 12 hours).

@misterdjules
Copy link
Contributor Author

@dougwilson All good thank you.

@dougwilson
Copy link
Contributor

@misterdjules just remembered I should have pinged you when the change landed on the master branch. It's on the master branch now :)

@misterdjules
Copy link
Contributor Author

@dougwilson Great, thank you for the heads up!

@expressjs expressjs locked and limited conversation to collaborators Sep 20, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants