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

test runner to include the name of the file being run #48457

Closed
mcollina opened this issue Jun 14, 2023 · 9 comments · Fixed by #48975
Closed

test runner to include the name of the file being run #48457

mcollina opened this issue Jun 14, 2023 · 9 comments · Fixed by #48975
Labels
feature request Issues that request new features to be added to Node.js. test_runner Issues and PRs related to the test runner subsystem.

Comments

@mcollina
Copy link
Member

What is the problem this feature will solve?

Let's consider the following output:

✔ logs errors during startup (121.652917ms)
✔ errors when starting an already started application (70.462041ms)
✔ errors when stopping an already stopped application (0.26525ms)
✔ does not restart while restarting (84.201625ms)
✔ restarts on SIGUSR2 (77.047292ms)
✔ stops on signals other than SIGUSR2 (38.43975ms)
✔ stops on uncaught exceptions (37.9015ms)
▶ supports configuration overrides
  ✔ throws on non-string config paths (30.867208ms)
  ✔ ignores invalid config paths (36.336958ms)
  ✔ sets valid config paths (46.613208ms)
▶ supports configuration overrides (114.128875ms)

✔ /Users/matteo/Repositories/platformatic/packages/runtime/test/cli/helper.mjs (98.878167ms)
✔ autostart (826.68275ms)
✔ start command (855.799541ms)
✖ handles startup errors (10102.117417ms)
  Error: Promise resolution is still pending but the event loop has already resolved
      at process.emit (node:events:513:28)

Unfortunately it's impossible to know at first glance where handles startup error is defined.
This is critical information for the user.

What is the feature you are proposing to solve the problem?

Whenever the test runner is running more than one file, we should print out the name of the file being run, either in the full path or relative to cwd, something like:

▶ path/to/my/test.js
  ✔ throws on non-string config paths (30.867208ms)
  ✔ ignores invalid config paths (36.336958ms)
  ✔ sets valid config paths (46.613208ms)

What alternatives have you considered?

No response

@mcollina mcollina added the feature request Issues that request new features to be added to Node.js. label Jun 14, 2023
@mcollina
Copy link
Member Author

cc @MoLow @cjihrig

@mcollina mcollina added the test_runner Issues and PRs related to the test runner subsystem. label Jun 14, 2023
@MoLow
Copy link
Member

MoLow commented Jun 14, 2023

that used to be the case, and it changed to avoid adding an indentation level when running with--test, and to avoid a big difference in the output when running with and without --test
I assume when you run the file directly without --test the output also doesn't include the name, so I would suggest adding the file name to the error message instead of adding an indentation level.
(maybe we can even add it only if the stack trace doesn't include it)

@mcollina
Copy link
Member Author

Adding it to the printed error would help a lot. Right now is not intuitive.

Aside from that, I think it's still confusing for me to not see where the tests are located.

Here is another option that keeps the indentation level:

>>> path/to/my/test.js

✔ throws on non-string config paths (30.867208ms)
✔ ignores invalid config paths (36.336958ms)
✔ sets valid config paths (46.613208ms)

Ultimately when running large test suites I care more about the file being run vs the individual test inside that file.
Maybe this can be a different reporter?

@cjihrig
Copy link
Contributor

cjihrig commented Jun 14, 2023

+1 to printing the filename. I also think it would be useful (although a different feature request) when printing the failures to print the full path of the test (for example top level test > subtest > another subtest > the actual test that failed).

@HinataKah0
Copy link
Contributor

If no one is currently working on it, I'll be happy to try... if it makes sense to implement this

I think we'll need to create a new event (e.g. test:file) to be emitted by runTestFile() in runner.js?
Technically, we can reuse test:start event with special nesting value (e.g. -1) but it seems like a hack IMO...

@cjihrig
Copy link
Contributor

cjihrig commented Jun 17, 2023

I think we'll need to create a new event

Could we add the filename to existing events instead of introducing a new one?

@MoLow
Copy link
Member

MoLow commented Jun 18, 2023

All existing events have a file property.
This issue is about the spec reporter and how it uses that emitted property.
I am ok with adding it but would really prefer something that bill behave the same when not using --test

@HinataKah0
Copy link
Contributor

HinataKah0 commented Jun 18, 2023

All existing events have a file property.

Yup...

Could we add the filename to existing events instead of introducing a new one?

Initially I was thinking of printing the filename when test:start is being emitted for the first time.
But it seems that there's no clear separation indicator between 2 files.
I received something like this (with enqueue and dequeue omitted):

<File 1>
test:start
test:pass
test:start
test:fail
<File 2>
test:start
test:pass
test:plan
test:diagnostic
...

Edit:
Wait... I have an assumption that we are printing the filename before each file run (not per test run) :)
Is it a bad assumption?

@mcollina
Copy link
Member Author

One more datapoint: it's almost impossible to know where a test failed with the current output on a large test suite:

✖ failing tests:

✖ handles startup errors (1296.021709ms)
  'Promise resolution is still pending but the event loop has already resolved'

✖ exits on error
  'Promise resolution is still pending but the event loop has already resolved'

✖ does not start if node inspector flags are provided
  'Promise resolution is still pending but the event loop has already resolved'

✖ starts the inspector
  'Promise resolution is still pending but the event loop has already resolved'
 ELIFECYCLE  Test failed. See above for more details.

cjihrig added a commit to cjihrig/node that referenced this issue Jul 30, 2023
This commit adds each test's line and column number to the reporter
output. This will aid in debugging test suite failures when error
stacks are not helpful, test suites are large, or tests have the
same name. This data is also exposed on the spec reporter.

This commit also replaces the filename that was previously being
reported, with the filename where the test actually exists. These
are normally correct, but could be wrong if tests were run from
a file other than the user's entrypoint.

Fixes: nodejs#48457
cjihrig added a commit to cjihrig/node that referenced this issue Aug 8, 2023
This commit adds each test's line and column number to the reporter
output. This will aid in debugging test suite failures when error
stacks are not helpful, test suites are large, or tests have the
same name. This data is also exposed on the spec reporter.

This commit also replaces the filename that was previously being
reported, with the filename where the test actually exists. These
are normally correct, but could be wrong if tests were run from
a file other than the user's entrypoint.

Fixes: nodejs#48457
cjihrig added a commit to cjihrig/node that referenced this issue Aug 10, 2023
This commit adds each test's line and column number to the reporter
output. This will aid in debugging test suite failures when error
stacks are not helpful, test suites are large, or tests have the
same name. This data is also exposed on the spec reporter.

This commit also replaces the filename that was previously being
reported, with the filename where the test actually exists. These
are normally correct, but could be wrong if tests were run from
a file other than the user's entrypoint.

Fixes: nodejs#48457
martenrichter pushed a commit to martenrichter/node that referenced this issue Aug 13, 2023
This commit adds each test's line and column number to the reporter
output. This will aid in debugging test suite failures when error
stacks are not helpful, test suites are large, or tests have the
same name. This data is also exposed on the spec reporter.

This commit also replaces the filename that was previously being
reported, with the filename where the test actually exists. These
are normally correct, but could be wrong if tests were run from
a file other than the user's entrypoint.

PR-URL: nodejs#48975
Fixes: nodejs#48457
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Ceres6 pushed a commit to Ceres6/node that referenced this issue Aug 14, 2023
This commit adds each test's line and column number to the reporter
output. This will aid in debugging test suite failures when error
stacks are not helpful, test suites are large, or tests have the
same name. This data is also exposed on the spec reporter.

This commit also replaces the filename that was previously being
reported, with the filename where the test actually exists. These
are normally correct, but could be wrong if tests were run from
a file other than the user's entrypoint.

PR-URL: nodejs#48975
Fixes: nodejs#48457
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Ceres6 pushed a commit to Ceres6/node that referenced this issue Aug 14, 2023
This commit adds each test's line and column number to the reporter
output. This will aid in debugging test suite failures when error
stacks are not helpful, test suites are large, or tests have the
same name. This data is also exposed on the spec reporter.

This commit also replaces the filename that was previously being
reported, with the filename where the test actually exists. These
are normally correct, but could be wrong if tests were run from
a file other than the user's entrypoint.

PR-URL: nodejs#48975
Fixes: nodejs#48457
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
RafaelGSS pushed a commit that referenced this issue Aug 15, 2023
This commit adds each test's line and column number to the reporter
output. This will aid in debugging test suite failures when error
stacks are not helpful, test suites are large, or tests have the
same name. This data is also exposed on the spec reporter.

This commit also replaces the filename that was previously being
reported, with the filename where the test actually exists. These
are normally correct, but could be wrong if tests were run from
a file other than the user's entrypoint.

PR-URL: #48975
Fixes: #48457
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
RafaelGSS pushed a commit to RafaelGSS/node that referenced this issue Aug 15, 2023
This commit adds each test's line and column number to the reporter
output. This will aid in debugging test suite failures when error
stacks are not helpful, test suites are large, or tests have the
same name. This data is also exposed on the spec reporter.

This commit also replaces the filename that was previously being
reported, with the filename where the test actually exists. These
are normally correct, but could be wrong if tests were run from
a file other than the user's entrypoint.

PR-URL: nodejs#48975
Fixes: nodejs#48457
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
rluvaton pushed a commit to rluvaton/node that referenced this issue Aug 15, 2023
This commit adds each test's line and column number to the reporter
output. This will aid in debugging test suite failures when error
stacks are not helpful, test suites are large, or tests have the
same name. This data is also exposed on the spec reporter.

This commit also replaces the filename that was previously being
reported, with the filename where the test actually exists. These
are normally correct, but could be wrong if tests were run from
a file other than the user's entrypoint.

PR-URL: nodejs#48975
Fixes: nodejs#48457
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
rluvaton pushed a commit to rluvaton/node that referenced this issue Aug 18, 2023
This commit adds each test's line and column number to the reporter
output. This will aid in debugging test suite failures when error
stacks are not helpful, test suites are large, or tests have the
same name. This data is also exposed on the spec reporter.

This commit also replaces the filename that was previously being
reported, with the filename where the test actually exists. These
are normally correct, but could be wrong if tests were run from
a file other than the user's entrypoint.

PR-URL: nodejs#48975
Fixes: nodejs#48457
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
rluvaton pushed a commit to rluvaton/node that referenced this issue Sep 4, 2023
This commit adds each test's line and column number to the reporter
output. This will aid in debugging test suite failures when error
stacks are not helpful, test suites are large, or tests have the
same name. This data is also exposed on the spec reporter.

This commit also replaces the filename that was previously being
reported, with the filename where the test actually exists. These
are normally correct, but could be wrong if tests were run from
a file other than the user's entrypoint.

PR-URL: nodejs#48975
Fixes: nodejs#48457
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
UlisesGascon pushed a commit that referenced this issue Sep 10, 2023
This commit adds each test's line and column number to the reporter
output. This will aid in debugging test suite failures when error
stacks are not helpful, test suites are large, or tests have the
same name. This data is also exposed on the spec reporter.

This commit also replaces the filename that was previously being
reported, with the filename where the test actually exists. These
are normally correct, but could be wrong if tests were run from
a file other than the user's entrypoint.

PR-URL: #48975
Backport-PR-URL: #49225
Fixes: #48457
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
targos pushed a commit that referenced this issue Nov 27, 2023
This commit adds each test's line and column number to the reporter
output. This will aid in debugging test suite failures when error
stacks are not helpful, test suites are large, or tests have the
same name. This data is also exposed on the spec reporter.

This commit also replaces the filename that was previously being
reported, with the filename where the test actually exists. These
are normally correct, but could be wrong if tests were run from
a file other than the user's entrypoint.

PR-URL: #48975
Fixes: #48457
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
sercher added a commit to sercher/graaljs that referenced this issue Apr 25, 2024
This commit adds each test's line and column number to the reporter
output. This will aid in debugging test suite failures when error
stacks are not helpful, test suites are large, or tests have the
same name. This data is also exposed on the spec reporter.

This commit also replaces the filename that was previously being
reported, with the filename where the test actually exists. These
are normally correct, but could be wrong if tests were run from
a file other than the user's entrypoint.

PR-URL: nodejs/node#48975
Fixes: nodejs/node#48457
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
sercher added a commit to sercher/graaljs that referenced this issue Apr 25, 2024
This commit adds each test's line and column number to the reporter
output. This will aid in debugging test suite failures when error
stacks are not helpful, test suites are large, or tests have the
same name. This data is also exposed on the spec reporter.

This commit also replaces the filename that was previously being
reported, with the filename where the test actually exists. These
are normally correct, but could be wrong if tests were run from
a file other than the user's entrypoint.

PR-URL: nodejs/node#48975
Fixes: nodejs/node#48457
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
@RedYetiDev RedYetiDev moved this from Awaiting Triage to Done in Node.js feature requests Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
4 participants