Add tap-spec option to npm test commands for mocha style reporting #363

danjm · 2018-09-26T18:44:47Z

This PR will allow us to see more details in the reports at the end of our test runs. In particular, we can now see a list of failed tests after all tests have run. This is done by adding a :tap-spec flag to any 'npm run test-name' command. The resulting output is formatted like the output of mocha's reporter. This is done using https://github.com/scottcorgan/tap-spec

For example, the current report from running npm run testBlockchainValid:tap-spec is

holgerd77

Thanks for the PR, to use this tool makes a lot of sense and should be super-useful. In the current form this bloats the list of scripts too much and introduces duplicity. Is there any reason not to add this directly to the original script commands. Otherwise I would prefer a lower-level solution like adding these to the dependencies (I am generally cautious on introducing new dependencies, this should be worth it though), and then just point in the README test section to the new possibility for summarized reports by adding the tap-spec pipe.

axic · 2018-09-27T10:51:32Z

The main point of tap is that the output can be streamed through formatters. One could just run npm run testAPI | tap-spec.

holgerd77 · 2018-09-27T10:53:49Z

Is tap-spec then emitting the same uncondensed output again? Then there should be nothing hindering to add this to the original commands, should it?

axic · 2018-09-27T10:55:05Z

No, tap-spec is doing the formatting already. One cannot run any other formatter on it afterwards.

holgerd77 · 2018-09-27T11:09:36Z

@danjm I think we should then just stick to the low-level dependency + README version.

danjm · 2018-09-28T02:37:04Z

@holgerd77 I made a significant change that attempts to incorporate your feedback while still adding an npm script to help developers.

The problem with just adding a dependency is that developers will have to specify the path to the executable via node_modules/.bin/. npm run testAPI | tap-spec will only work if tap-spec is installed globally.

If we want to keep things really simple we could just put a note in the README about globally installing reporters, a link to some recommended reporters and an example of piping to it. This would avoid installing another dependency. I think that is a fine approach.

Alternatively, I just pushed commit ffe689a

It only adds one npm script, which can receive a script name or path as an argument. It pipes to tap-spec by default, but can receive an argument that specifies a different reporter. It does this via a node script that I added to a new scripts/ directory.

The additions made to the README in the commit explain the usage in more detail, with examples.

holgerd77

Hi Dan, I am generally ok with this approach, thanks for the rework!

Should we also add this to the CircleCI testState... commands and would the feedback mechanism for success/failure back to GitHub still work (cc @axic, @jwasinger)? I would like this a lot to always have a comprised test report at the end of a test run, this "not ok" full-text search is really not optimal! 😄

holgerd77 · 2018-10-02T11:29:51Z

package.json

@@ -17,13 +17,14 @@
    "testBlockchain": "npm run build:dist && node --stack-size=1500 ./tests/tester -b --dist --excludeDir='GeneralStateTests'",
    "testBlockchainGeneralStateTests": "npm run build:dist && node --stack-size=1500 ./tests/tester -b --dist --dir='GeneralStateTests'",
    "testBlockchainBlockGasLimit": "npm run build:dist && node --stack-size=1500 ./tests/tester -b --dist --dir='bcBlockGasLimitTest'",
-    "testBlockchainValid": "npm run build:dist && node --stack-size=1500 ./tests/tester -b --dist --dir='bcValidBlockTest'",
+    "testBlockchainValid": "npm run build:dist && node --stack-size=1500 ./tests/tester -b --dist --dir='bcValidBlockTest' | tap-spec",


This needs to be removed.

Removed in 39cc0d5

holgerd77 · 2018-10-02T11:30:28Z

package.json

@@ -60,6 +61,8 @@
    "minimist": "^1.1.1",
    "nyc": "^12.0.2",
    "standard": "^10.0.0",
+    "tap-mocha-reporter": "^3.0.7",


tap-mocha-reporter isn't needed as a dependency here, or is it?

You're right, it's not needed.

Removed in 39cc0d5

holgerd77 · 2018-10-02T11:34:05Z

scripts/formatTest.js

+    'testBlockchainTotalDifficulty',
+    'testAPI',
+    'test'
+  ]


Is this really necessary to duplicate the script names here or could this be done in a more general way? Would we loose much if we just allow this for every command and if it is not there it just breaks?

Hi @danjm, can you give a short opinion on the two questions above?

It is there so that we can distinguish which tests need to be run with npm run and which with node.

One approach we could take is to read the npm scripts dynamically.

d9b4ea9 changes scripts/formatTest.js so that it reads directly from package.json. This removes the need to hard code any script names and make any changes to the file if/when package.json is changed.

holgerd77 · 2018-10-02T11:40:04Z

Ah, and please also rebase on the way.

holgerd77 · 2018-10-05T11:56:58Z

Any updates here?

danjm · 2018-10-11T17:37:54Z

@holgerd77 Apologies for the delay. I will be incorporating your requested changes later this evening. I have answered your above two questions.

…e or path as args.

…p-spec PR.

… package.json scripts.

danjm · 2018-10-11T22:52:01Z

@holgerd77 I have made changes in response to your comments and have rebased onto the latest master.

holgerd77

Ok, great, this now looks good, will merge.

holgerd77 · 2018-10-15T11:19:51Z

One note: we use linting on all our libraries for a unified code formatting and it is good to accommodate to always run a last "npm run lint" before pushing (now directly fixed a linting error, forgotten semicolon), I often forget this myself though. 😄

holgerd77 requested changes Sep 27, 2018

View reviewed changes

holgerd77 requested changes Oct 2, 2018

View reviewed changes

holgerd77 added the PR state: needs changes label Oct 2, 2018

danjm added 4 commits October 11, 2018 20:21

Add tap-spec option to npm test commands for mocha style reporting

3911327

Replace test specific tap-spec scripts with one to receive script nam…

25d6a51

…e or path as args.

Clean up unnecessary changes to package.json from the tests-option-ta…

8464428

…p-spec PR.

Generalize/futureproof formatTest.js against any changes/additions to…

610cc1a

… package.json scripts.

danjm force-pushed the tests-option-tap-spec branch from d9b4ea9 to 610cc1a Compare October 11, 2018 22:51

holgerd77 added 2 commits October 15, 2018 13:05

Merge branch 'master' into tests-option-tap-spec

a16f22e

Fix linting

76618dd

holgerd77 added PR state: needs review and removed PR state: needs changes labels Oct 15, 2018

holgerd77 approved these changes Oct 15, 2018

View reviewed changes

holgerd77 merged commit e9b60a1 into ethereumjs:master Oct 15, 2018

evertonfraga added the package: vm label Feb 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tap-spec option to npm test commands for mocha style reporting #363

Add tap-spec option to npm test commands for mocha style reporting #363

danjm commented Sep 26, 2018

holgerd77 left a comment

axic commented Sep 27, 2018

holgerd77 commented Sep 27, 2018

axic commented Sep 27, 2018

holgerd77 commented Sep 27, 2018

danjm commented Sep 28, 2018

holgerd77 left a comment

holgerd77 Oct 2, 2018

danjm Oct 11, 2018

holgerd77 Oct 2, 2018

danjm Oct 11, 2018

danjm Oct 11, 2018

holgerd77 Oct 2, 2018

holgerd77 Oct 11, 2018

danjm Oct 11, 2018

danjm Oct 11, 2018

holgerd77 commented Oct 2, 2018

holgerd77 commented Oct 5, 2018

danjm commented Oct 11, 2018

danjm commented Oct 11, 2018

holgerd77 left a comment

holgerd77 commented Oct 15, 2018

Add tap-spec option to npm test commands for mocha style reporting #363

Add tap-spec option to npm test commands for mocha style reporting #363

Conversation

danjm commented Sep 26, 2018

holgerd77 left a comment

Choose a reason for hiding this comment

axic commented Sep 27, 2018

holgerd77 commented Sep 27, 2018

axic commented Sep 27, 2018

holgerd77 commented Sep 27, 2018

danjm commented Sep 28, 2018

holgerd77 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerd77 commented Oct 2, 2018

holgerd77 commented Oct 5, 2018

danjm commented Oct 11, 2018

danjm commented Oct 11, 2018

holgerd77 left a comment

Choose a reason for hiding this comment

holgerd77 commented Oct 15, 2018