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: Convert file URLs to paths in test enqueue method #51615

Closed
wants to merge 9 commits into from

Conversation

mertcanaltin
Copy link
Member

The enqueue method of the test runner now converts file URLs to file paths before emitting test events. This change ensures consistency in file representations and resolves parsing issues on platforms like Windows.

  • Convert file URLs to paths in the enqueue method
  • Emit file paths instead of file URLs in test events
  • Improve platform compatibility and consistency in file handling

#51610

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Jan 31, 2024
@mertcanaltin mertcanaltin requested review from MoLow, cjihrig and mcollina and removed request for cjihrig January 31, 2024 13:18
@mertcanaltin mertcanaltin changed the title feat: Convert file URLs to paths in test enqueue method test_runner: Convert file URLs to paths in test enqueue method Jan 31, 2024
@aduh95
Copy link
Contributor

aduh95 commented Jan 31, 2024

Should we consider using URLs everywhere instead? In case we want to support test input that are not in the file system (e.g. HTTPS modules)

Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test?

@mcollina
Copy link
Member

Can you please add a test that validates that all URLs produced are parseable? Note that I had to do https://github.com/mcollina/borp/blob/c7c99c1c70afcbd13f903b678fbbec7c821e72fc/lib/reporters.js#L6-L14 to parse this. I think this currently fails on Windows.

I generically prefer paths because #51609 and writing tests is more complicated if the implementation of the conversion is system dependent.

@mertcanaltin
Copy link
Member Author

I added a test, so sorry for my delay ❤️

@mertcanaltin mertcanaltin requested review from anonrig and MoLow February 7, 2024 08:19
@MoLow
Copy link
Member

MoLow commented Feb 11, 2024

please check test failures

@rluvaton
Copy link
Member

Isn't this a breaking change?
Also docs should be updated

@mertcanaltin
Copy link
Member Author

instead of the old solution I made a normalizeFilePath method and replaced the path with file if
process.platform === 'win32' @mcollina Is it healthy to control win32 ?

@GeoffreyBooth
Copy link
Member

@MoLow @anonrig Do you think it would be good to fix these tests or should I focus on my code?

What are the test failures, for at least a few of the representative ones?

I’m suspicious about the several module- and filesystem-related failures. If this PR changes something core to Node about how file paths/file URLs are handled, and that’s resulting in broken tests in systems beyond the test runner, that smells like a bug being introduced. I’m sympathetic to the desire to see more useful representation of test files in test runner output, but not at the cost of a breaking change that affects core stuff.

@anonrig anonrig added the needs-citgm PRs that need a CITGM CI run. label Feb 18, 2024
@cjihrig
Copy link
Contributor

cjihrig commented Mar 24, 2024

The linked issue is now closed, so I'll go ahead and close this as well. Thanks for the PR though.

@cjihrig cjihrig closed this Mar 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. needs-citgm PRs that need a CITGM CI run. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants