Unit test cleanup + partial makefile linting#10048
Open
intrigus-lgtm wants to merge 6 commits into
Open
Conversation
intrigus-lgtm
requested review from
emwang-jump,
lidatong and
ripatel-fd
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a Makefile linter to catch silent $(call ...) typos and missing run-unit-test registrations, and updates various Local.mk files to satisfy/enforce these checks.
Changes:
- Introduced
contrib/lint/check_makefiles.pyand amake linttarget to validateLocal.mkcall-macro names and ensure built unit tests are registered to run (with allowlists/annotations for exceptions). - Enabled running a number of unit tests by default via new
$(call run-unit-test,...)lines and added# lint-no-run-unit-testannotations where tests must remain non-default. - Wired the new lint into GitHub Actions (
lint.yml) and invoked it on pull requests.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vinyl/Local.mk | Annotates a unit test to intentionally not run by default. |
| src/util/wksp/Local.mk | Adds lint opt-out annotation for a unit test and adjusts run list. |
| src/util/tpool/Local.mk | Registers test_tpool to run by default. |
| src/util/tmpl/Local.mk | Registers additional template/data-structure tests to run by default. |
| src/util/simd/Local.mk | Fixes typo in unit-test run macro invocation. |
| src/util/shmem/Local.mk | Registers test_shmem to run by default. |
| src/util/sandbox/Local.mk | Registers test_sandbox to run by default. |
| src/util/net/Local.mk | Annotates a unit test to intentionally not run by default. |
| src/util/log/Local.mk | Registers test_log to run by default. |
| src/util/bits/Local.mk | Registers an additional bits test to run by default. |
| src/tango/cnc/Local.mk | Annotates a unit test to intentionally not run by default. |
| src/flamenco/runtime/Local.mk | Registers test_hashes to run; annotates another test to not run by default. |
| src/flamenco/log_collector/Local.mk | Annotates a unit test to intentionally not run by default. |
| src/flamenco/accdb/Local.mk | Registers test_accdb_v0 to run by default. |
| src/discof/restore/Local.mk | Annotates a unit test to intentionally not run by default. |
| src/discof/forest/Local.mk | Annotates a unit test to intentionally not run by default. |
| src/disco/keyguard/Local.mk | Registers test_keyload to run by default. |
| src/choreo/eqvoc/Local.mk | Annotates a unit test to intentionally not run by default. |
| src/choreo/Local.mk | Registers test_choreo_base to run by default. |
| src/ballet/sbpf/Local.mk | Annotates a unit test to intentionally not run by default. |
| src/ballet/reedsol/Local.mk | Registers test_reedsol to run by default. |
| src/ballet/pb/Local.mk | Registers test_pb to run by default. |
| src/ballet/lthash/Local.mk | Registers test_lthash to run by default. |
| src/ballet/hmac/Local.mk | Fixes typo in unit-test run macro invocation. |
| src/ballet/bigint/Local.mk | Fixes typo in header macro invocation (add-hdrs). |
| src/ballet/aes/Local.mk | Registers test_aes to run by default. |
| contrib/lint/check_makefiles.py | Adds the new Local.mk linter (call-name + make→run checks). |
| config/everything.mk | Adds lint phony/target and documents it in make help. |
| .github/workflows/on_pull_request.yml | Adds a lint workflow call on non-draft PRs. |
| .github/workflows/lint.yml | New workflow that runs make lint in CI. |
| $(call run-unit-test,test_map_slot,) | ||
| $(call run-unit-test,test_map_slot_para,) | ||
| # FIXME: MAP_PERFECT? | ||
| $(call run-unit-test,test_map_perfect,) |
This was referenced Jun 3, 2026
intrigus-lgtm
force-pushed
the
intrigus/feat/unit-test-cleanup
branch
from
4f5ccb8 to
d074995
Compare
ripatel-fd
reviewed
Jun 3, 2026
| $(call run-unit-test,test_wksp_admin) | ||
| $(call run-unit-test,test_wksp_user) | ||
| #$(call run-unit-test,test_wksp_helper) # FIXME: why was this not enabled? | ||
| $(call run-unit-test,test_wksp_tpool) |
Contributor
There was a problem hiding this comment.
This unit test wastes too much CPU on one tile, should decrease --iter-max or use multiple tiles (in script-tests)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I was recently PRing some fixes for forest and realized that the tests were broken even without my fix.
Which made me realize that the test wasn't run in CI at all...
There have been quite a few times we missed tests in the past (just a sample):
#8319
#8194
#5640
#1139
This PR adds linting for makefiles:
$(call add-hdr,fd_uint256.h fd_uint256_mul.h)(should be add-hrds) or when there are typos. I didn't find any other good way to prevent this without custom linting.Linting for 2. can be disabled using
# lint-no-run-unit-test(e.g. if the test required CLI args).In total, 21 tests were re-enabled. I disabled four of those again as they are broken and will be fixed in follow-up PRs.