test_runner: add TAP parser

[manekinekko]

This PR adds initial support for a TAP LL(1) parser. This implementation is based on the grammar for TAP14 from https://testanything.org/tap-version-14-specification.html

TODO:

• add a TAP checker (by design, the current parser does only parsing).
• add parallel tests for the TAP lexer
• add parallel tests for the TAP parser
• add parallel tests for the TAP checker
• add async parsing
• add more js docs
• integrate the new parser into the existing node:test runner implementation (shoutout to @MoLow for their help)
• fix linting errors (make lint)

Refs: #43344
Signed-off-by: Wassim Chegham [email protected]

@benjamincburns @fhinkel @cjihrig

[cjihrig]

@manekinekko is there any update on this? I think this is probably the highest priority item for the test runner at this point.

[MoLow]

I think this is probably the highest priority item for the test runner at this point.

+1.
if there is any help needed with this LMK.

additionaly - will this solution support arbitrary console.log statements? #44372

[manekinekko]

Sorry for the delay guys. Had to deliver some work lately.

So, the biggest challenge I need to work on solving right now is redesigning the lexer so it can support async scanning and make the parser support partial parsing, and emit the results once they are available (as discussed with @cjihrig over Twitter).

A draft of a public API I am thinking of would look something like this:

  const parser = new TapParser(/*  probably a flag to enable stream support */);
  child.stdout.on('data', (chunk) => {
   parser.parse(chunk);
  });
  child.stdout.on('end', () => {
   const status = parser.end();
  assert.strictEqual(status.ok, true);
  });

@MoLow @cjihrig do you have any ideas about a different design / or a concern?

[cjihrig]

Is the lexer right now reading individual characters? If so, I think we can leverage the fact that TAP is line based. That would make it significantly simpler than a parser for something like a programming language. I think that would also make it simpler to handle streaming.

[manekinekko]

The parser is already splitting tokens (scanned by the lexer) into subsets, separated by EOL. The resulting array basically contains all tokens scanned at each TAP line.

from   [ token1, EOL, token2, EOL, ..., EOL, tokenN-1, tokenN ]
to     [ [token1], [token2], [...], [tokenN-1, tokenN] ]

So I was thinking of leveraging this logic for streams. is that was you were referring to?

[cjihrig]

I guess I'm just wondering if we need a full blown lexer. Would it be simpler (less code, faster for us to get something shipped) to read input, turn it into lines, and parse each line. There are only a handful of line types outside of yaml blocks, and we can generally distinguish them by looking at just the first few characters of the line (whitespace, 'ok', 'not ok', 'TAP version', etc.).

[MoLow]

@cjihrig how would you expect a console.log or any invalid tap to behave?

[MoLow]

we can generally distinguish them by looking at just the first few characters of the line (whitespace, 'ok', 'not ok', 'TAP version', etc.).

There is something vere similar here https://github.com/nodejs/tap2junit just FYI

[cjihrig]

@cjihrig how would you expect a console.log or any invalid tap to behave?

According to the spec:

Any output that is not a version, a plan, a test line, a YAML block,
a diagnostic or a bail out is incorrect. How a harness handles the
incorrect line is undefined. Test::Harness silently ignores incorrect
lines, but will become more stringent in the future. TAP::Harness
reports TAP syntax errors at the end of a test run.

and

In this document, the “harness” is any program analyzing TAP
output. Typically this will be Perl’s runtests program, or the underlying
TAP::Harness-runtests> method. A harness must only read TAP
output from standard output and not from standard error. Lines written
to standard output matching /^(not )?ok\b/ must be interpreted as test
lines. After a test line a block of lines starting with ‘—’ and ending with
‘…’ will be interpreted as an inline YAML document providing extended
diagnostic information about the preceding test. All other lines must not
be considered test output.

I think if there are extra lines, that might come from console.log()s, we should try to keep them but definitely not error because users would not be able to add logging statements in their tests. If the TAP lines themselves are malformed I think we should report an error (unless I missed something about this in the spec, in which case I defer to the spec).

[manekinekko]

I agree with @cjihrig on the specs. The parser in this PR will error if a line starts with an unrecognized token. Pragmas are parsed but not yet applied.

Could we print those console.logs as comments?

[MoLow]

I think it is ok to print invalid tap as a comment or something else, but we should not just ignore it

[manekinekko]

@cjihrig @MoLow wdyt about this API?

  const args = ['--test', testFixtures];
  const child = spawn(process.execPath, args);

  const parser = new TapParser();          //<-- create a parser instance

  child.stdout.on('data', (chunk) => {
    const line = chunk.toString('utf-8');
    const test = parser.parseChunk(line);  //<--- call parseChunk() 
    console.log(test);
  });

The test output can be either:

1. undefined (when we parse non-testpoint entries like pragmas or comments)
2. or the last parsed test node (extracted from the AST):

{
  "status": { fail: false, pass: true, todo: false, skip: false },
  "id": "1",
  "description": "/home/wassimchegham/oss/@nodejs/node/test/fixtures/test-runner/index.test.js",
  "reason": "",
  diagnostics: [ 'duration_ms: 0.033821156', 'duration_ms: 0.033821156' ]
}

[MoLow]

@manekinekko I would expect something based on node:readline, like:

class TapParser extends readline.InterfaceConstructor {
	constructor(input, output) {
      super(input, output);
      this.on('line', (line) => {
		..parse the line and
		this.emit('test') / this.emit('diagnostic') etc
	  })
    }
}

either that or

class TapParser extends TapStream {
	constructor(input) {
      this.handle = readline.createInterface({ input });
      this.handle.on('line', (line) => {
		..parse the line and
		this.ok(data) / this.fail(data) etc
	  })
    }
}

[MoLow]

that is obviously very simplistic since TAP parsing can include multilines, etc

[nodejs-github-bot]

Landed in f8ce911

[manekinekko]

OMG!! That was one hell of a ride!! Thanks y'all for taking the time to review and comment on this work!

Special Thank You to @cjihrig @MoLow @fhinkel @benjamingr for your help and for being such great mentors ❤️

Already looking forward to my next contributions 👍

[ljharb]

cc @nodejs/testing