Skip to content

feat: split long running UTs#1631

Open
arnaubennassar wants to merge 2 commits into
developfrom
feat/split-test-short-long
Open

feat: split long running UTs#1631
arnaubennassar wants to merge 2 commits into
developfrom
feat/split-test-short-long

Conversation

@arnaubennassar
Copy link
Copy Markdown
Collaborator

@arnaubennassar arnaubennassar commented May 28, 2026

🔄 Changes Summary

  • Split unit tests that are "pure" vs the ones that have deps that make them slow

⚠️ Breaking Changes

None

📋 Config Updates

None

✅ Testing

tested that things keep working... UTs still take 20s to run

🐞 Issues

@claude
Copy link
Copy Markdown

claude Bot commented May 28, 2026 •
edited
Loading

Claude finished @arnaubennassar's task in 1m 17s —— View job

Review Complete ✅

I've reviewed this PR that splits unit tests into fast and slow categories. The implementation is clean and follows Go testing best practices.

🚀 What's New

  • Added new test-unit-short target in Makefile that skips slow integration tests using the -short flag
  • Added testing.Short() guards to 22 integration tests across 13 files to enable fast test runs
  • Tests are now properly categorized: fast unit tests vs slower integration tests that use simulated backends, Docker, or external dependencies

🐛 Bug Fixes

None.

📋 Config Updates

None.

⚠️ Breaking Changes

None.

Review Feedback

✅ Strengths

  1. Consistent Implementation: All 22 tests follow the exact same pattern with proper skip messages

  2. Clean Makefile Addition: The new test-unit-short target is well-documented and follows existing conventions

  3. Correct Test Selection: The tests marked for skipping are truly integration tests that:

    • Use simulated.Backend for blockchain simulation
    • Require Docker/Geth containers
    • Have network dependencies or RPC calls
    • Take significantly longer to run

  4. Backward Compatibility: The existing make test-unit command remains unchanged, so CI/CD pipelines continue to work

🟢 Observations

Test Coverage Breakdown:

  • aggoracle: 2 E2E tests
  • bridgesync: 5 E2E/integration tests
  • claimsync: 4 simulated backend tests
  • etherman: 1 upgrade test
  • l1infotreesync: 3 E2E tests
  • l2gersync: 3 E2E tests with legacy contract support
  • pprof: 1 HTTP server test
  • reorgdetector: 6 integration tests

Performance Impact: Based on the PR description mentioning "UTs still take 20s to run," the fast tests complete quickly while the full suite with integration tests remains comprehensive.

🟡 Suggestions (Optional)

  1. Documentation Enhancement: Consider adding a comment in the Makefile explaining when to use test-unit-short vs test-unit:

    # Use test-unit-short for rapid feedback during development
# Use test-unit for comprehensive testing before commits

  2. CI Integration: You might want to add a fast CI job that runs make test-unit-short on every commit for quick feedback, keeping the full test suite for PR validation.

  3. Editor Integration Hint: The Makefile comment mentions "suitable for editor on-save runs" which is excellent - consider adding this to the development docs.

✅ Code Quality

  • Consistency: Perfect - all tests use identical guard pattern
  • Test Semantics: Correct - testing.Short() is the idiomatic Go approach for this
  • Skip Messages: Clear and descriptive
  • No Logic Changes: The tests themselves remain untouched, only guards added

Conclusion

This is a well-executed PR that improves developer experience by enabling fast test iterations. The implementation is clean, consistent, and follows Go best practices. The categorization of tests is accurate, and the approach is maintainable.

Recommendation: ✅ Approve

@arnaubennassar arnaubennassar self-assigned this May 28, 2026
@arnaubennassar arnaubennassar changed the title split long running UTs feat: split long running UTs May 29, 2026
@arnaubennassar @claude
fix: move t.Parallel() before t.Skip() to satisfy tparallel linter
6782dd9 
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 29, 2026

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

joanestebanr
joanestebanr reviewed Jun 2, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@joanestebanr joanestebanr left a comment

Choose a reason for hiding this comment

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

I suggest that CI also runs the short test individually for this reason:

  • If unit-test fails will be so easy to known if are the long or short ones
  • To prevent a non-short test from being mistakenly classified as short.

Comment thread Makefile
@@ -105,6 +105,10 @@ build-docker-debug: ## Builds a debug docker image (dlv headless on :40000, no o
test-unit: ## Runs the unit tests
trap '$(STOP)' EXIT; MallocNanoZone=0 go test -count=1 -short -race -p 1 -covermode=atomic -coverprofile=coverage.out -timeout 15m ./...
Copy link
Copy Markdown
Collaborator

@joanestebanr joanestebanr Jun 2, 2026

Choose a reason for hiding this comment

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

Makefile target test-unit also runs the short version of tests (-short)

Comment thread Makefile
trap '$(STOP)' EXIT; MallocNanoZone=0 go test -count=1 -short -race -p 1 -covermode=atomic -coverprofile=coverage.out -timeout 15m ./...

.PHONY: test-unit-short
test-unit-short: ## Runs only the fast unit tests (skips integration tests). Suitable for editor on-save runs.
Copy link
Copy Markdown
Collaborator

@joanestebanr joanestebanr Jun 2, 2026

Choose a reason for hiding this comment

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

Maybe make sense to use the same params as test-unit? or we want to force that short ones can run in parallel? WDYT:

trap '$(STOP)' EXIT; MallocNanoZone=0 go test -count=1 -short -race -p 1 -covermode=atomic -coverprofile=coverage.out -timeout 15m ./...

But decreasing the timeout to prevent that any no-short test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joanestebanr joanestebanr joanestebanr left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@arnaubennassar arnaubennassar

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[FEATURE] Split unittest between short and long

2 participants

@arnaubennassar @joanestebanr