Skip to content

test: drop bookkeeping for GC-observation tests deleted in #35182 (+0 tests, cleanup only) - #35402

Open
cirospaciari wants to merge 4 commits into
mainfrom
claude/cleanup-deleted-gc-test-bookkeeping
Open

test: drop bookkeeping for GC-observation tests deleted in #35182 (+0 tests, cleanup only)#35402
cirospaciari wants to merge 4 commits into
mainfrom
claude/cleanup-deleted-gc-test-bookkeeping

Conversation

@cirospaciari

Copy link
Copy Markdown
Member

#35182 deleted test-gc-http-client-connaborted.js, test-net-connect-memleak.js and test-tls-connect-memleak.js, but left their bookkeeping behind: the expectations.txt FLAKY entry for the tls test and the three expected-durations.json entries. This removes the stale entries; no behavior change (the referenced tests no longer exist). Surfaced by review on #34654.

The three tests were removed, but their expectations.txt FLAKY entry
and expected-durations.json entries stayed behind.
@robobun

robobun commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator
Updated 10:17 PM PT - Aug 3rd, 2026

@robobun, your commit 17f49b0 has 3 failures in Build #88584 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 35402

That installs a local version of the PR into your bun-35402 executable, so you can run:

bun-35402 --bun

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Changes

Test metadata updates

Layer / File(s) Summary
Quarantine and duration metadata
test/expectations.txt, test/expected-durations.json
Removes the LINUX-X64-MUSL flaky expectation for the TLS connection memory-leak test and applies whitespace-only edits to expected-duration entries.

Possibly related PRs

  • oven-sh/bun#34176: Directly overlaps with removal of the same MUSL quarantine entry.
  • oven-sh/bun#34848: Updates quarantine behavior for another Node parallel test in the same expectations file.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description states the change, but it misses the required verification section and template headings. Add the template headings and include a concise note under 'How did you verify your code works?'.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly matches the main change: removing stale test bookkeeping for deleted GC-observation tests.

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

Found 1 issue this PR may fix:

  1. CI: test-net-connect-memleak.js fails on half of PR builds on linux-x64-musl since June 28 ~23:00 UTC #33044 - Removes the stale FLAKY annotation and expected-duration entry for test-net-connect-memleak.js, the exact test tracked by this issue, completing the cleanup after Delete flaky GC-observation node tests #35182 deleted the test file

If this is helpful, copy the block below into the PR description to auto-close this issue on merge.

Fixes #33044

🤖 Generated with Claude Code

Comment thread test/expectations.txt Outdated
@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. process/worker: env descriptor validation, worker execArgv policy table with per-worker --expose-gc (+3 tests, worker 74%→76%) #34654 - Superset that deletes the same three test files and removes the same stale bookkeeping entries from expectations.txt and expected-durations.json

🤖 Generated with Claude Code

@cirospaciari cirospaciari changed the title test: drop bookkeeping for GC-observation tests deleted in #35182 test: drop bookkeeping for GC-observation tests deleted in #35182 (+0 tests, cleanup only) Jul 24, 2026
…d-gc-test-bookkeeping

# Conflicts:
#	test/expectations.txt
#	test/expected-durations.json

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

After merging main (057ac83), this PR now has zero net diff — the stale expectations.txt entry was removed on main by #36780 and the expected-durations.json entries by a00c4db, so there's nothing left to land here. The orphaned-comment issue I flagged earlier is moot. This can be closed.

Extended reasoning...

Overview

This PR originally removed stale bookkeeping (a FLAKY entry in test/expectations.txt and three test/expected-durations.json entries) for GC-observation tests deleted in #35182. My prior review on 2026-07-24 flagged that the expectations.txt hunk left an 11-line explanatory comment block orphaned mid-sentence.

Since then, commit 057ac83 merged origin/main into the branch. git diff e3368190..HEAD (main tip vs. PR HEAD) is now empty — no files differ. The cleanup this PR set out to do landed on main independently: #36780 ("test: prune stale entries from expectations.txt") removed the TLS-memleak FLAKY entry and its full comment block, and a00c4db ("Update test durations") regenerated expected-durations.json without the deleted tests. I also confirmed the three test files no longer exist and no stale references remain in expectations.txt or expected-durations.json.

Security risks

None — no code changes remain.

Level of scrutiny

Minimal; the PR is a no-op after the merge.

Other factors

I'm deferring rather than approving because merging an empty PR would add a merge commit with no effect; the author will likely want to close it instead. My earlier orphaned-comment finding no longer applies since the entire hunk is gone.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM — after the latest merge from main this PR carries an empty diff. The cleanup it targeted (the test-tls-connect-memleak.js FLAKY entry + the three expected-durations.json entries) already landed on main via #36780 and a durations regen, so there is nothing left to merge here. My earlier note about the orphaned comment block is moot — that block is gone on main. This can be closed or merged as a no-op.

Extended reasoning...

Overview

This PR originally removed stale bookkeeping in test/expectations.txt and test/expected-durations.json for three GC-observation tests that #35182 deleted. Since my previous review it has merged origin/main twice (057ac83, 17f49b0). Comparing the branch HEAD against its main-side merge parent (git diff HEAD^2 HEAD) yields zero changes — main already picked up equivalent cleanup via #36780 (expectations prune) and a subsequent expected-durations.json regeneration.

Security risks

None. Test-metadata only, and the net diff is empty.

Level of scrutiny

Minimal. An empty diff cannot introduce a regression. I verified: the three test files (test-gc-http-client-connaborted.js, test-net-connect-memleak.js, test-tls-connect-memleak.js) do not exist on the branch, test/expectations.txt no longer references them or contains the orphaned FinalizationRegistry comment block I flagged on Jul 24, and grepping test/ for the three names finds only an unrelated comment in test/js/node/test/common/gc.js.

Other factors

My prior inline comment about the truncated comment block is marked resolved and is now moot — the whole block was removed on main. Since this PR contributes nothing beyond what main already has, approving is safe; the author can close it or let it merge as a no-op.

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

Successfully merging this pull request may close these issues.

2 participants