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: move 'http' tests into subfolder #52875

Closed
wants to merge 5 commits into from

Conversation

RedYetiDev
Copy link
Member

This PR moves all of the parallel/test-http-* files into parallel/http/test-*. If this change goes well, I look to implement it throughout the entire parallel folder (one subsystem at a time)

IMO, the DX will be much easier with this change, as searching through tests will become simpler, as users can simply visit the test/parallel/<subsystem> directory.

From a maintenance point of view, CODEOWNERS and label rules should be simpler, as a subdirectory now exists for these subsystems.

The http subsystem is largest subsystem (in number of test files), so I decided to handle it first.

@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 May 7, 2024
@RedYetiDev RedYetiDev added http Issues or PRs related to the http subsystem. review wanted PRs that need reviews. labels May 7, 2024
@RedYetiDev
Copy link
Member Author

This PR is a step towards parity with #52859

(CC Original participants: @targos @MoLow)

Copy link
Member

@pimterry pimterry left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@targos
Copy link
Member

targos commented May 8, 2024

Does it work well with the parallel.status file?

@RedYetiDev
Copy link
Member Author

I updated the file, so it should work, but I haven't done hard testing

@targos
Copy link
Member

targos commented May 8, 2024

Can you still run only "http" tests with ./tools/test.py http ?

@RedYetiDev
Copy link
Member Author

Can you still run only "http" tests with ./tools/test.py http ?

It seems not, but I'll build a fix for that now

@RedYetiDev
Copy link
Member Author

RedYetiDev commented May 8, 2024

Okay, now it is possible :-)

@RedYetiDev RedYetiDev removed the review wanted PRs that need reviews. label May 8, 2024
@RedYetiDev
Copy link
Member Author

RedYetiDev commented May 8, 2024

@targos i'll test the parallel.status file now

@RedYetiDev
Copy link
Member Author

parallel.status works fine :-). I forgot to push it, and now that it's pushed, it works.

@RedYetiDev RedYetiDev requested a review from targos May 8, 2024 14:57
@targos
Copy link
Member

targos commented May 8, 2024

@nodejs/http @nodejs/testing

@RedYetiDev RedYetiDev added the request-ci Add this label to start a Jenkins CI on a PR. label May 8, 2024
@RedYetiDev
Copy link
Member Author

RedYetiDev commented May 8, 2024

Requesting a CI, as this PR has two approves, and a CI is probably going to be needed before merge.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 8, 2024
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

I'm somewhat concerned that this would make backporting commits harder. I think we would need backport PRs done immediately for all lines (18, 20) before landing this.

@RedYetiDev
Copy link
Member Author

I'm somewhat concerned that this would make backporting commits harder. I think we would need backport PRs done immediately for all lines (18, 20) before landing this.

Okay, I'll open backport PRs soon.

@RedYetiDev RedYetiDev added the blocked PRs that are blocked by other issues or PRs. label May 8, 2024
@RedYetiDev
Copy link
Member Author

RedYetiDev commented May 8, 2024

This will add the initial support: #52901

@@ -112,4 +112,4 @@ test-inspector-async-hook-setup-at-inspect-brk: PASS, FLAKY

[$arch==loong64]
# https://github.com/nodejs/node/issues/51662
test-http-correct-hostname: SKIP
http/test-correct-hostname: SKIP
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
http/test-correct-hostname: SKIP
http/test-correct-hostname: SKIP

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll make these changes (and others) once this is unblocked

@tniessen
Copy link
Member

FWIW, many tests do not fit cleanly into any particular subsystem, and I don't see how we'd consistently apply test/parallel/<subsystem> in those cases.

There are almost 500 open PRs. Aside from backporting, we would need to ensure that PRs such as this one are not going to cause mass conflicts in dozens of other PRs.

I am not in favor, but also not strongly opposed, assuming all relevant git and GitHub operations keep referring to the actual changes and not to the mass renames.

@RedYetiDev
Copy link
Member Author

It doesn't need to be consistently applied, tests not put in a folder will run the same way. The behavior that i would hope to be implemented in the blocking PR would allow for tests to appear in sub folders.

As for the mass conflicts, I'm aware of the wave of them that will come, but I'm prepared to handle it, because I think this will really do some good

@RedYetiDev
Copy link
Member Author

Closing until blocking PR is merged, as the merge conflicts will only increase.

@RedYetiDev RedYetiDev closed this Jun 3, 2024
@RedYetiDev RedYetiDev deleted the test-http-subfolder branch June 3, 2024 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked PRs that are blocked by other issues or PRs. http Issues or PRs related to the http subsystem. 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.

8 participants