Skip to content

balancergroup: merge whitebox tests into blackbox test#9094

Open
hugehoo wants to merge 1 commit into
grpc:masterfrom
hugehoo:balancegroup-test-migration
Open

balancergroup: merge whitebox tests into blackbox test#9094
hugehoo wants to merge 1 commit into
grpc:masterfrom
hugehoo:balancegroup-test-migration

Conversation

@hugehoo
Copy link
Copy Markdown
Contributor

@hugehoo hugehoo commented Apr 25, 2026

fixes #8996

  • this PR covers test isolation in internal/balancergroup packages
  • merges white box test (balancergroup/balancergroup_test.go) into black box test (balancergroup/balancergroup_ext_test.go) to ensure to test the API, not the internal symbols.
    • moved 16 tests from balancergroup_test.go to balancergroup_ext_test.go
  • no production code changed

RELEASE NOTES: N/A

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 25, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.02%. Comparing base (3d82ab3) to head (88ac5b6).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9094      +/-   ##
==========================================
- Coverage   83.08%   83.02%   -0.07%     
==========================================
  Files         413      413              
  Lines       33484    33484              
==========================================
- Hits        27821    27799      -22     
- Misses       4240     4250      +10     
- Partials     1423     1435      +12     

see 19 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Pranjali-2501 Pranjali-2501 self-requested a review April 28, 2026 04:00
@Pranjali-2501
Copy link
Copy Markdown
Contributor

Hi @hugehoo, Thanks for raising the PR.

Instead of creating a new file and copy the content, can you please use git mv to move the file and then update the package name to a test only package.

@hugehoo
Copy link
Copy Markdown
Contributor Author

hugehoo commented Apr 29, 2026

Hi @hugehoo, Thanks for raising the PR.

Instead of creating a new file and copy the content, can you please use git mv to move the file and then update the package name to a test only package.

thanks for the comment, didn't know such a mechanism.
applied on 42d8fab

@Pranjali-2501 Pranjali-2501 requested a review from easwars May 4, 2026 11:29
@Pranjali-2501 Pranjali-2501 assigned easwars and unassigned hugehoo May 4, 2026
@easwars
Copy link
Copy Markdown
Contributor

easwars commented May 4, 2026

If you are simply moving the tests from one file to another and changing the package name, why is the diff still so huge? I'm not entirely convinced whether the git mv was done appropriately.

@hugehoo
Copy link
Copy Markdown
Contributor Author

hugehoo commented May 4, 2026

@easwars the git mv is meaningless in this case because balancergroup_ext_test.go already exists on master. Git can detect a rename when the target file doesn't exist but since it does, git recognizes this as:

  • balancergroup_test.go: entire file deleted (847 lines)
  • balancergroup_ext_test.go: modified from 124 lines to ~930 lines

No matter how I re-structure the commits, the "Files changed" will show the same large diff, i guess.


Although It would still get a large diff, I could reset current commits and restructure 3 small commits to make it easier to review.

  1. Consolidate ext_test into balancergroup_test.go (+82 / -124): moves TestBalancerGroup_RemoveImmediately into the
  whitebox test file and deletes balancergroup_ext_test.go
  2. Rename test file (0 lines changed): pure git mv, detected as R100
  3. Convert to blackbox (+18 / -17): mechanical changes — package name, import, and balancergroup. prefix

@easwars
Copy link
Copy Markdown
Contributor

easwars commented May 5, 2026

@hugehoo : Sorry, I didnt realize that the file existed already. But feel free to create a new file, so that the diff only ends up showing the lines that are actually changed.

@easwars easwars assigned hugehoo and unassigned easwars May 5, 2026
Convert balancergroup_test.go in-place from whitebox (package
balancergroup) to blackbox (package balancergroup_test) testing.
This minimizes the PR diff by keeping the same filename and only
changing the lines that need to differ for external package access.

Changes:
- package balancergroup → package balancergroup_test
- Add imports: "strings", "google.golang.org/grpc/internal/balancergroup"
- Qualify types: New → balancergroup.New, Options → balancergroup.Options,
  BalancerGroup → balancergroup.BalancerGroup, ParseConfig → balancergroup.ParseConfig
- defaultTestShortTimeout: 10ms → 100ms (avoid flakes)
- Move TestBalancerGroup_RemoveImmediately from ext_test into main test file
- Delete balancergroup_ext_test.go (no longer needed)
@hugehoo hugehoo force-pushed the balancegroup-test-migration branch from 42d8fab to 88ac5b6 Compare May 5, 2026 04:55
@hugehoo
Copy link
Copy Markdown
Contributor Author

hugehoo commented May 5, 2026

@easwars thanks for comments!
I removed the balancergroup_ext_test.go and updated whitebox balancegroup_test.go to blackbox.
Moved the blackbox tests(balancergroup_ext_test.go) into balancegroup_test.go too. So the file name haven't changed and git diff only shows where actually changed.
88ac5b6

@easwars easwars assigned easwars and unassigned hugehoo May 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

balancergroup: move as many tests as possible to a test-only package

3 participants