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

index.js: Fallback to require(...) for unrecognized reporters #48

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

wking
Copy link

@wking wking commented Jun 7, 2018

This allows us to use reporters that aren't built in. For example, with this commit you can:

$ npm install mochawesome
$ tap --reporter=mochawesome ...

Fixes #46.

Catching up with:

* Mocha, which has had these as part of its suites since
  mochajs/mocha@c0bb9188 (plugging in Hook, 2011-11-25), and

* Mochawesome, which has expected arrays since at least
  adamgruber/mochawesome@5a49d410 (3.0, 2017-11-30,
  adamgruber/mochawesome#214).

Without this change, Mochawesome's:

  [].concat(suite._beforeAll, suite._beforeEach)

will lead to:

  [undefined, undefined]

and Mochawesome will call cleanTest on those undefined values and
crash.
@wking wking force-pushed the require-reporter-fallback branch 2 times, most recently from d48e15f to d76f4da Compare June 7, 2018 23:39
wking added 6 commits June 8, 2018 02:30
Catching up with mochajs/mocha@30582e64 (if a reporter has a .done
method, call it before exiting, 2014-05-17, mochajs/mocha#1218).
Mochawesome has used:

  skipped = (!cleaned.pass && !cleaned.fail && !cleaned.pending);

since adamgruber/mochawesome@2d4a63b7 (logic to record skipped tests,
2015-01-29).

Do not emit 'test end' for skips or TODO (pending) tests, because
Mochawesome has:

  obj.stats.skipped = obj.stats.testsRegistered - obj.stats.tests;

since that same commit, so we need these tests entered in
testsRegistered (via the 'test' event) but not in tests (via the 'test
end' event).

Do not emit 'pass' for skips either, otherwise we'll end up with
failures + passes > tests.

Also fix "pass" -> "passed" for .state.  Mochawesome has been
comparing the value to "passed" in its cleaner since
adamgruber/mochawesome@d687633d (Initial commit, 2014-07-11), and
Mocha itself has been comparing with "passed" since at least
mochajs/mocha@2c720a35 (do not eat exceptions thrown asynchronously
from passed tests, 2018-02-28, mochajs/mocha#3257).
This is, as far as I know, Mochawesome-specific, which makes it a bit
of a hack.  The Mochawesome side of this is
adamgruber/mochawesome@65e90621 (implement addContext method for
adding report context to tests, 2016-12-20,
adamgruber/mochawesome#106).  Ideally we'd want a way to trigger this
as part of requesting Mochawesome as the reporter, but the addition
seems harmless enough so I'm just including it every time.

In the diag.source case, we might want to expose diagnostics besides
.fn.  But we surely don't want to expose the test source via both .fn
and .context.  For this commit, I've left the diag.source case alone,
but in future work we might want something closer to :

  if (result.diag) {
    var context = Object.assign({}, result.diag)
    if (context.source) {
      var source = context.source
      delete context.source
      this.fn = {
        toString: function () {
          return 'function(){' + source + '\n}'
        }
      }
    }
    if (Object.keys(context).length) {
      this.context = {
        title: 'diagnostic',
        value: context,
      }
    }
  }

I haven't done that here, because Object.assign is not supported on
Internet Exporer, Android webview, or Opera for Android [1], despite
being part of ECMAScript 2015 [2].  Object.keys seems to be supported
everywhere [3].  We may only care about Node for this package, but I'm
being conservative for this commit.

[1]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign#Browser_compatibility
[2]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign#Specifications
[3]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/keys#Browser_compatibility
Skips like [1]:

  ok 23 # skip Insufficient flogiston pressure.

are parsed (at least by [email protected]) into structures like:

  {
    id: 1,
    name: "",
    ok: true,
    skip: "root.readonly is false but the root filesystem is still not writable"
  }

But title-less tests are not very useful.  With this change, cases
like the above empty-string name will fall back to the skip value.
And if that skip value is undefined, we'll fall back to an empty
string (because I'm not sure how well downstream consumers would
handle and undefined title).

One positive effect of this change is that Mochawesome now has a title
message to render for these skips (where previously it just used an
empty h4).

[1]: https://testanything.org/tap-version-13-specification.html#skipping-tests
Mochawesome has expected a suite attached to the runner since
adamgruber/mochawesome@e25a5eb0 (handle nested describes, 2014-07-14).
This commit creates a new root Suite for that, and attaches the
per-TAP suites as children of the root suite.  This will allow us to
render output from multiple TAP files with Mochawesome.

I've also refactored the ancestory properties:

* Test.parent became Test.suite, since that's the only object type
  we're using there.

* Suite.parent now only references other Suites.  There's no need to
  complicate the ancestry by including both Suites and Parsers, and
  sticking to suites means we don't need Parser methods for title
  chaining.

The new titlePath methods catch us up with mochajs/mocha@aa24e82f
(indent test contexts, 2017-05-20, mochajs/mocha#2814).  I've left
fullTitle around for now for compat with older versions of Mocha.
This allows us to use reporters that aren't built in.  For example,
with this commit you can:

  $ npm install
  $ npm install --no-save mocha mochawesome
  $ cp index.js node_modules/tap-mocha-reporter/
  $ cp lib/*.js node_modules/tap-mocha-reporter/lib/
  $ npm test -- --reporter=mochawesome

That's installing [email protected] and [email protected].
@wking wking force-pushed the require-reporter-fallback branch from d76f4da to e4f8183 Compare June 8, 2018 09:35
wking added a commit to wking/tap-mocha-reporter that referenced this pull request Jun 8, 2018
Generated with:

  $ node_modules/.bin/tap --reporter=mochawesome /tmp/test1.sh /tmp/test2.sh

for [1].

[1]: tapjs#48
@wking
Copy link
Author

wking commented Jun 8, 2018

I've pushed a number of other commits to this branch to get Mochawesome working. With the current tip (e4f8183), you can test with two dummy TAP generators:

$ cat /tmp/test1.sh 
#!/bin/sh
cat <<EOF
TAP version 13
ok 1 # SKIP testing a skip
not ok 2 - can multiply one by two
  ---
  {
    "actual": 3,
    "expected": 2
  }
  ...
ok 3 - can multiply two by two
  ---
  {
    "actual": 4,
    "expected": 4
  }
  ...
1..3
EOF
$ cat /tmp/test2.sh 
#!/bin/sh

cat <<EOF
TAP Version 13
1..2
# nesting
    1..2
    ok 1 - true is ok
    not ok 2 - doag is also okay
not ok 1 - nesting
ok 2 - second
EOF

Install tap and clobber the npm-installed tap-mocha-reporter with the content from this branch:

$ npm install
$ npm install --no-save mocha mochawesome
$ cp index.js node_modules/tap-mocha-reporter/
$ cp lib/*.js node_modules/tap-mocha-reporter/lib/

Test this repository, feeding the output into Mochawesome:

$ npm test -- --reporter=mochawesome

Test the dummy generators, feeding the output into Mochawesome:

$ node_modules/.bin/tap --reporter=mochawesome /tmp/test1.sh /tmp/test2.sh

For folks who are curious, but not curious enough to run the above themselves ;), you can see the generated output here while this PR is in flight.

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

Successfully merging this pull request may close these issues.

1 participant