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: add tmpdir.fileURL() #49040

Merged
merged 1 commit into from
Aug 10, 2023

Conversation

LiviaMedeiros
Copy link
Contributor

@LiviaMedeiros LiviaMedeiros commented Aug 6, 2023

Almost every time tmpdir.path is used in tests, we have to import path to use it.
Whenever we need URL of something in tmpdir, we also have to import url.pathToFileURL.
Adding tmpdir.url would reduce

import { join } from 'node:path';
import { pathToFileURL } from 'node:url';
const testFile = pathToFileURL(join(tmpdir.path, 'subdir', 'tst-file-123'));

to

const testFile = tmpdir.resolve('subdir', 'tst-file-123');

or to native resolving

const tmpBase = tmpdir.fileURL();
const testFile = new URL('subdir/tst-file-123', tmpBase);

Additionally, maybe we could add path-oriented tmpdir.resolve(...paths) to replace path.join(tmpdir.path, ...paths) in tests?

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Aug 6, 2023
@tniessen
Copy link
Member

tniessen commented Aug 6, 2023

Why is the URL not a URL?

@LiviaMedeiros
Copy link
Contributor Author

Why is the URL not a URL?

For consistency with import.meta.url and general guideline. No strong opinion here.

@tniessen
Copy link
Member

tniessen commented Aug 6, 2023

I see. For anyone else wondering the same thing, it sounds like the main issue is that URL is meant to be mutable, which is undesirable for values that are not meant to be modified.

@aduh95
Copy link
Contributor

aduh95 commented Aug 8, 2023

We could have tmpdir.fileURL() which would return a new URL instance, and would accept a relative URL as optional parameter, that would be consistent with fixtures.mjs. wdyt?

@joyeecheung
Copy link
Member

Considering we also already have fixtures.fileURL() and fixtures.path() I think tmpdir.fileURL() and tmpdir.resolve() (because tmpdir.path is already taken) would make a lot of sense (I've always wondered why we don't have tmpdir.resolve() whenever I have to do path.join(tmpdir.path))

@LiviaMedeiros LiviaMedeiros changed the title test: add tmpdir.url test: add tmpdir.fileURL() Aug 9, 2023
@LiviaMedeiros LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 9, 2023
@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 9, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 10, 2023
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 10, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 10, 2023
@nodejs-github-bot nodejs-github-bot merged commit 7bbcb29 into nodejs:main Aug 10, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 7bbcb29

martenrichter pushed a commit to martenrichter/node that referenced this pull request Aug 13, 2023
PR-URL: nodejs#49040
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#49040
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#49040
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
PR-URL: #49040
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@UlisesGascon UlisesGascon mentioned this pull request Aug 15, 2023
RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Aug 15, 2023
PR-URL: nodejs#49040
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
rluvaton pushed a commit to rluvaton/node that referenced this pull request Aug 15, 2023
PR-URL: nodejs#49040
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Aug 17, 2023
PR-URL: #49040
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
nodejs-github-bot pushed a commit that referenced this pull request Aug 21, 2023
PR-URL: #49138
Refs: #49040
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
PR-URL: #49138
Refs: #49040
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Nov 11, 2023
PR-URL: nodejs#49040
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@targos targos added the backported-to-v18.x PRs backported to the v18.x-staging branch. label Nov 23, 2023
targos pushed a commit that referenced this pull request Nov 23, 2023
PR-URL: #49040
Backport-PR-URL: #50669
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
targos pushed a commit that referenced this pull request Nov 27, 2023
PR-URL: #49138
Refs: #49040
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#49040
Backport-PR-URL: nodejs/node#50669
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#49138
Refs: nodejs/node#49040
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#49040
Backport-PR-URL: nodejs/node#50669
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#49138
Refs: nodejs/node#49040
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backported-to-v18.x PRs backported to the v18.x-staging branch. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants