Add tags = ["manual"] to all test/orfs targets#9741
Add tags = ["manual"] to all test/orfs targets#9741oharboe wants to merge 10 commits intoThe-OpenROAD-Project:masterfrom
Conversation
bazelisk build //... was building test/orfs artifacts that should only be built when explicitly running tests. Add manual tag to all 60 targets that were missing it (filegroups, sh_tests, orfs_flow test_kwargs) so that wildcard patterns skip them entirely. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully implements the new tagging policy for test/orfs/ targets. By adding tags = ["manual"] to various filegroup, orfs_flow, and sh_test rules, and updating test_kwargs to include both manual and orfs tags, the change ensures that bazelisk build //... will no longer build ORFS test artifacts. The README.md has also been updated to clearly document this policy and provide verification steps. The changes are consistent and correctly applied across all modified files, achieving the stated objective.
|
I don't see the tests running in the CI now |
ouch. will fix and reopen |
The previous commit accidentally set all test targets to tags = ["manual"], dropping the "orfs" tag that CI uses to select these tests. Restore ["manual", "orfs"] on all test targets while keeping non-test targets as ["manual"] only. Add CI checks to prevent this regression: verify all test targets have the orfs tag, all non-test targets don't, and all targets have manual. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
|
I still don't see the tests running in the CI |
Prevents `bazelisk test //...` from running ORFS integration tests tagged with "orfs". Combined with the existing `build --build_tag_filters=-orfs`, this ensures wildcard test invocations skip test/orfs targets. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Replace GitHub Actions query checks with a Starlark macro verify_orfs_tags() that validates tags when BUILD files are loaded. Any bazelisk command touching test/orfs packages will fail immediately if a target violates the policy: - All targets must have tags = ["manual"] - Test targets must also have "orfs" tag - Non-test targets must NOT have "orfs" tag Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
How is it invoked from CI? Should be fixed now. |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
you can just look at pr-head and see not working |
The manual tag excludes targets from all wildcard expansion (//...), so CI's `bazelisk test //...` never discovers these tests. Test targets need only the orfs tag -- the test_tag_filters mechanism in .bazelrc handles the local vs CI distinction. Non-test targets (filegroups, orfs_run, orfs_flow build targets) keep tags = ["manual"] so `bazelisk build //...` never builds them. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
CI uses --config=ci. The default test --test_tag_filters=-orfs skips orfs-tagged tests locally, but CI needs to run them. Adding test:ci --test_tag_filters= clears the filter for CI builds. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Test targets: tags = ["orfs"] (no "manual"), so //... finds them and test_tag_filters controls local vs CI behavior. Non-test targets: tags = ["manual"] (no "orfs"), so build //... never builds ORFS artifacts. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Reflect the new policy: test targets use tags = ["orfs"] (not manual), non-test targets use tags = ["manual"]. Document the local vs CI behavior controlled by test_tag_filters in .bazelrc. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
The previous commit missed test_kwargs dict values (different syntax from kwarg tags) and sim_test calls where tags weren't the last argument before the closing paren. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
eqy_flow_test() generates test_suite targets with a _tests suffix (e.g. Element_eqy_4x4_tests), but the eqy_tests test_suite was referencing names without the suffix. This was a latent bug hidden by the manual tag, exposed when manual was removed from test targets. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
|
clang-tidy review says "All clean, LGTM! 👍" |
|
I think this will just lead to developers getting a false signal as the CI will run a different set of tests than they will (by default). I'm not trying to turn off tests, just not have them run as part of build. |
ah. So you do want bazelisk test ... to test test/orfs/... ? |
|
revisit after #9827 |
2 participants
bazelisk build //... was building test/orfs artifacts that should only be built when explicitly running tests. Add manual tag to all 60 targets that were missing it (filegroups, sh_tests, orfs_flow test_kwargs) so that wildcard patterns skip them entirely.