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

All tests in repo fail if any test in repo has an extraneous /*--- sequence #161

Open
ben-allen opened this issue Jun 9, 2023 · 2 comments

Comments

@ben-allen
Copy link

ben-allen commented Jun 9, 2023

how to replicate:

  1. Break a test by adding an extra /*---. In this case, I've broken type-datetimefield-valid.js in intl402/DisplayNames/prototype/of.
// Copyright 2021 the V8 project authors. All rights reserved.
// This code is governed by the BSD license found in the LICENSE file.
/*---
/*---
esid: sec-Intl.DisplayNames.prototype.of
description: Returns string value for valid `dateTimeField` codes
features: [Intl.DisplayNames-v2]
---*/

var displayNames = new Intl.DisplayNames(undefined, {type: 'dateTimeField'});
[...]
  1. Run any test anywhere in the repo. The following error message happens:
test262-harness --host-type=jsc --host-path=`which jsc` locales-symbol-length.js 
/usr/local/lib/node_modules/test262-harness/node_modules/test262-stream/lib/test-file.js:65
      throw new Error(`Error loading frontmatter from file ${test262File.file}\n${e.message}`);
      ^

Error: Error loading frontmatter from file test/intl402/DisplayNames/prototype/of/type-datetimefield-valid.js
end of the stream or a document separator is expected at line 3, column 5:
    esid: sec-Intl.DisplayNames.prototype.of
        ^
    at loadAttrs (/usr/local/lib/node_modules/test262-harness/node_modules/test262-stream/lib/test-file.js:65:13)
    at extractAttrs (/usr/local/lib/node_modules/test262-harness/node_modules/test262-stream/lib/test-file.js:81:17)
    at new Test262File (/usr/local/lib/node_modules/test262-harness/node_modules/test262-stream/lib/test-file.js:143:18)
    at builder (/usr/local/lib/node_modules/test262-harness/node_modules/test262-stream/lib/builder.js:16:16)
    at /usr/local/lib/node_modules/test262-harness/node_modules/test262-stream/lib/compile.js:16:9
    at FSReqCallback.readFileAfterClose [as oncomplete] (node:internal/fs/read_file_context:68:3)
@catamorphism
Copy link

I spent some time looking into this. The bug, or at least surprising behavior, is that test262-harness parses the metadata out of all the files in the current working directory, even if you pass in one relative filename like in Ben's example. Normally, this is not visible to the user, because it suppresses output from tests that don't match the pattern(s) provided on the command line. But if a test has a syntax error in its metadata, like type-datetimefield-valid.js did in Ben's example, then you get an error message about a file that you didn't pass as an argument to test262-harness. This is surprising.

Why does this happen? test262-harness depends on the test262-stream library, which supplies a TestStream constructor that takes an array of paths and returns a readable stream, on which it emits one event per test file that it reads. If a file has a syntax error (or doesn't exist, etc.) it emits an error on the stream. In the TestStream class in test262-harness itself (which wraps the upstream TestStream), the error handler passes through any upstream errors it receives. This means that the entire stream fails if a single error is emitted upstream. I figured this out through printf-debugging: the main run.js file composes two streams, the first one of which (the pipe() call on line 175) invokes the test262-stream library to get all the needed paths, generating a stream of test objects; the second one of which (the pipe() call on line 179) actually runs each test. Because there was an error in the first stream, nothing in the second stream ever ran.

It's not obvious to me what the right solution is. Some possibilities:

  1. Only pass specific filenames to test262-stream (this call), not entire directories (unless the user actually specified the whole directory on the command line). The comment on patternsToDirectories() in test-stream.js explains pretty well why this isn't already being done. However, it would be more robust for test262-harness to expand any globs and pass the resulting filenames to test262-stream rather than passing a superset of the desired files by passing in an enclosing parent directory for anything containing globs.
  2. Alternately, the error handler in test-stream.js (here) could be modified to ignore errors that relate to files the user didn't ask to test. This would require modifying the test262-stream library as well to throw a more structured exception type with a property specifying the path of the offending file.
  3. A hack to avoid changing the exception type would be to do the same thing, but just have the aforementioned error handler scrape out the filename from error.message, which would work at least in the situation Ben ran into.
  4. Or, the error diagnostics could simply be improved to highlight the filenames with different colors, or to print a list of files being read at the beginning... not sure what the right UX is, but anything that helps explain why you're getting an error for type-datetimefield-valid.js when you only wanted to run locales-symbol-length.js.

I was going to try to fix this, until I realized it wasn't obvious what the right solution was.

@catamorphism
Copy link

In any case, the bug title should probably be something more general, like "syntax error in one test causes all tests in the same directory to fail."

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

No branches or pull requests

2 participants