When a catastrophic error happens the output of tap-arc is success

[telmotrooper]

Describe the issue
When a catastrophic error happens the output of tap-arc is success. It should be an error, just as if a test failed.

Steps to reproduce
Steps to reproduce the behavior:

1. Add anything to a test that would throw a catastrophic error such as an invalid import:
   const { hello } = require('./this-is-a-fake-path')
2. Run tap-arc and check how even though you get errors, the output still returns success, which would make a pipeline step assume nothing went wrong with the tests.

Expected behavior
When a catastrophic error happens, the output of tap-arc should indicate failure (return 1).

Screenshots

(Green arrow in Oh My Zsh indicates successful execution of last command, while red indicates failure.)

Desktop
Please complete the following information (if appropriate):

• OS: Manjaro Linux
• Version: 0.3.3

[tbeseda]

Good catch, thanks for reporting!

I did some research and interestingly there isn't a great way to get the exit code (or any sort of state) from the previous command in a pipeline. Seems like the safe thing to do is assume that if there are zero test results, something is busted and we should exit 1. Looks like this is how other reporters handle this case.

I'll release 0.3.4 shortly with the fix.

[Powersource]

Been running into a few situations with the latest tap-arc where tape exits with 1 but tap-arc exits with 0.

% npm t

> @ssb-graphql/artefact@4.2.2 test
> npm run test:js && npm run lint


> @ssb-graphql/artefact@4.2.2 test:js
> npm run test:raw | tap-arc


> @ssb-graphql/artefact@4.2.2 test:raw
> tape -r esm test/**/*.test.js


saveArtefact (create + get)
    ✓ save artefact type document doesnt throw errors
/home/me/prj/ssb/ssb-graphql-artefact/node_modules/@apollo/client/errors/errors.cjs:31
        var _this = _super.call(this, errorMessage) || this;
                           ^

ApolloError: Abstract type "Artefact" must resolve to an Object type at runtime for field "Query.artefact". Either the "Artefact" type should provide a "resolveType" function or each possible type should provide an "isTypeOf" function.
    at new ApolloError (/home/me/prj/ssb/ssb-graphql-artefact/node_modules/@apollo/client/errors/errors.cjs:31:28)
    at /home/me/prj/ssb/ssb-graphql-artefact/node_modules/@apollo/client/core/core.cjs:1843:47
    at both (/home/me/prj/ssb/ssb-graphql-artefact/node_modules/@apollo/client/utilities/utilities.cjs:1072:53)
    at /home/me/prj/ssb/ssb-graphql-artefact/node_modules/@apollo/client/utilities/utilities.cjs:1065:72
    at new Promise (<anonymous>)
    at Object.then (/home/me/prj/ssb/ssb-graphql-artefact/node_modules/@apollo/client/utilities/utilities.cjs:1065:24)
    at Object.next (/home/me/prj/ssb/ssb-graphql-artefact/node_modules/@apollo/client/utilities/utilities.cjs:1073:49)
    at notifySubscription (/home/me/prj/ssb/ssb-graphql-artefact/node_modules/zen-observable/lib/Observable.js:135:18)
    at onNotify (/home/me/prj/ssb/ssb-graphql-artefact/node_modules/zen-observable/lib/Observable.js:179:3)
    at SubscriptionObserver.next (/home/me/prj/ssb/ssb-graphql-artefact/node_modules/zen-observable/lib/Observable.js:235:7) {
  graphQLErrors: [
    {
      message: 'Abstract type "Artefact" must resolve to an Object type at runtime for field "Query.artefact". Either the "Artefact" type should provide a "resolveType" function or each possible type should provide an "isTypeOf" function.',
      locations: [ { line: 2, column: 3 } ],
      path: [ 'artefact' ]
    }
  ],
  protocolErrors: [],
  clientErrors: [],
  networkError: null,
  extraInfo: undefined
}

Node.js v18.16.0

total:     2
passing:   2
3.147 s


> @ssb-graphql/artefact@4.2.2 lint
> standard --fix

me@me-lap[ssb/ssb-graphql-artefact]% echo $?
0

[Powersource]

I guess the difference is that in my case a few tests actually pass at first and so the 0-test check doesn't get triggered.

[tbeseda]

Apologies, I've been neglecting my GH notifications.
I'll re-open this and have a look!

[filmaj]

You can see this in action in https://github.com/architect/parser/actions/runs/14297200895/job/40065885797?pr=229
I think this is a pretty serious bug, as it can make it seem like everything is working fine.

[filmaj]

I swapped out tap-arc in the above PR for faucet and now the PR correctly fails: architect/parser#229, so there must be something in the way that tap-arc handles piped stdin vs. the way that faucet does that is causing this issue.

[filmaj]

I spent a bit more time looking into this.

First, why does faucet work for the PR I mentioned in my last comment? It's more coincidence than intention: faucet relies on a very old version of tap-parser - v0 line - which is from ~11 years ago. The above PR has code that throws an exception before any t.plan calls, and that violates an assumption that tap-parser places on tests: that each test should call t.plan, which causes it to emit an error.

This comment from @tbeseda is correct, in fact I'd go further: there is no way to get pipe process state, at least not in the context of an npm run script:

I did some research and interestingly there isn't a great way to get the exit code (or any sort of state) from the previous command in a pipeline.

The only way to do so, AFAICT, is to wrap a pipe in a bash script and check for the PIPESTATUS environment variable. This is not really doable within a package.json run script, unless the run script invokes a bash script. IMO this is another failure of the tape testing project, where it explicitly recommends (see bottom of the "pretty reporters" section of its docs) to use pipes with reporters.

The 'new' version of tape, tapjs, gets around this by providing a --reporter flag for specifying a reporter from the main tap process. This allows tapjs to control both processes (the test runner as well as the reporter), and so can catch exceptions happening in the test runner process. Some reporters also get around this by doing the reverse: faucet, for example, accepts a list of test files as arguments and will spawn the tape process itself, exactly for the purpose of being able to catch the test runner exit code.

Stepping back for a moment: as a maintainer of architect, I don't think it is worth it for us to maintain a tap reporter. There are already so many repos to keep on top of in this organization and not enough help to go around. The test reporter is not part of the main value Architect intends to provide. Additionally, with node 18 EOL in two weeks, starting in node 20 we can move to the native node test runner. I am in favour of doing that if only to remove dependencies and having much faster test run times. I have a PoC PR up in the parser repo (architect/parser#230) that makes this move, and it is much faster (~order of magnitude, not to mention less dependencies). The native node test runner even supports a tap reporter out of the box, so possibly someone who wanted to continue maintaining this reporter could still leverage it in conjunction with the new native node test runner, though this repo would likely need changes to interface with the node native test custom reporter interface (basically a specific kind of Stream, which is a really nice abstraction!).

[tbeseda]

I tend to agree with you here @filmaj

I think maintaining any sort of parser like this is a worthy endeavor because it's definitely useful. Unfortunately the complexity here is probably out of scope for the Arc org. Especially given the movement forward in Node v20+.

btw, thanks for the research and writeup