Clean up test suite: remove dead code, fix race conditions, centralize helpers by openroad-ci · Pull Request #332 · The-OpenROAD-Project/OpenSTA

openroad-ci · 2026-04-03T03:32:49Z

Summary

Remove 19 comment-only C++ test stubs from TestSearchStaInit.cc, TestSearchStaDesign.cc, TestSearchStaDesignB.cc
Centralize assert_file_nonempty / assert_file_contains to test/helpers.tcl, remove 17 inline definitions in verilog tests
Remove read_sdc roundtrip sections from 20 SDC tests to fix ctest -j30 file-level race condition (two tests sharing sdc_exc_override1.sdc)
Delete redundant graph_make_verify test (covered by graph_advanced)
Delete committed log artifacts (network_gcd_traversal.log, util_report_redirect.log)
Add */test/*.log and Testing/ to .gitignore
Fix unquoted variable expansions in test/regression.sh
Fix stray " in etc/Build.sh

Regression

6087/6087 tests pass (ctest -j$(nproc))

- Remove temp files: network_gcd_traversal.log, util_report_redirect.log - Delete 19 comment-only C++ test stubs (dead code for removed APIs) - Remove redundant graph_make_verify test (covered by graph_advanced) - Centralize assert_file_nonempty/assert_file_contains into test/helpers.tcl and remove inline copies from 17 verilog test files - Fix Build.sh stray quotes in heredoc output - Fix regression.sh unquoted variable expansions - Update .gitignore: add */test/*.log and Testing/ All 6087 tests pass. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

…-cleanup Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

…ate/OpenSTA into secure-test-suite-cleanup Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

The merge-base changed after approval.

jhkim-pii · 2026-04-03T05:23:08Z

@maliberty

The merge-base changed after approval.

Could you please approve again?

The merge-base changed after approval.

dsengupta0628

Hi Jaehyun-before I add review for this issue, can you please make sure OpenROAD compilation goes through with the new STA changes? I was trying to update OpenROAD's src/sta to latest OpenSTA head with a PR, when I saw that OpenROAD fails.

I can reproduce the compilation errors locally by setting the flags to treat Warnings as errors (as done in Jenkins). Example below (use this command to compile from OpenROAD ./etc/Build.sh -clean -no-warnings -no-gui )

/workspace/localrepos/orfs_merge_lvf_sta/tools/OpenROAD/src/sta/liberty/test/cpp/TestLibertyStaCallbacks.cc: In function ‘void sta::expectStaLibertyCoreState(Sta*, LibertyLibrary*)’:
/workspace/localrepos/orfs_merge_lvf_sta/tools/OpenROAD/src/sta/liberty/test/cpp/TestLibertyStaCallbacks.cc:45:6: error: suggest explicit braces to avoid ambiguous ‘else’ [-Werror=dangling-else]
45 | if (!sta->scenes().empty())
| ^
/workspace/localrepos/orfs_merge_lvf_sta/tools/OpenROAD/src/sta/liberty/test/cpp/TestLibertyStaBasicsB.cc: In function ‘void sta::expectStaLibertyCoreState(Sta*, LibertyLibrary*)’:
/workspace/localrepos/orfs_merge_lvf_sta/tools/OpenROAD/src/sta/liberty/test/cpp/TestLibertyStaBasicsB.cc:43:6: error: suggest explicit braces to avoid ambiguous ‘else’ [-Werror=dangling-else]
43 | if (!sta->scenes().empty())
| ^
/workspace/localrepos/orfs_merge_lvf_sta/tools/OpenROAD/src/sta/network/test/cpp/TestNetworkB.cc: In member function ‘virtual void sta::ConcreteNetworkLinkedTest_MakeTermAndTermName_Test::TestBody()’:
/workspace/localrepos/orfs_merge_lvf_sta/tools/OpenROAD/src/sta/network/test/cpp/TestNetworkB.cc:1538:10: error: unused variable ‘tnet_check’ [-Werror=unused-variable]
1538 | Net *tnet_check = network_.net(term);

dsengupta0628

This is a well-structured cleanup PR that reduces test suite maintenance burden without losing coverage.

Eliminates real parallel test hazards: the read_sdc roundtrip blocks shared temp files, which is a legitimate race condition under ctest -j
Consolidates before removing: dedicated roundtrip tests (sdc_write_roundtrip.tcl, sdc_write_roundtrip_full.tcl) already exist, so the scattered copies were truly redundant
Helper centralization is clean: 17 duplicate proc definitions collapsed into test/helpers.tcl which was already being sourced
Dead C++ stubs (19 empty test bodies) were pure noise inflating test counts
Shell script fixes are genuine bug fixes (heredoc stray quotes, unquoted variables)
Committed log artifacts removed and properly gitignored

jhkim-pii · 2026-04-03T23:37:16Z

@dsengupta0628 Thank you for the guide. I will check and clean it up.

…leanup Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

jhkim-pii · 2026-04-05T15:23:37Z

@dsengupta0628
Thank you for fixing the OR build issue.

By the way, if OpenROAD CI uses -Werror, how about using it for OR/OpenSTA build?
Then OpenSTA build will catch the warnings early.

I made the change in CMakeLists.txt.

…ate/OpenSTA into secure-test-suite-cleanup Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

The remove_constraints API was permanently removed from Sta. Delete the commented-out calls, TODO comments, and "skipped" print statements instead of keeping placeholder blocks. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

Previously these functions returned 1 on mismatch but no caller checked the return value, allowing roundtrip failures to go undetected. Now they call error() so the sta process exits non-zero and the regression runner catches the failure. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

The valid field name is "net" (singular) per search/Search.tcl. Using "nets" triggered Warning 168 (unknown field) silently in 29 test scripts. Fix the field name and regolden .ok files. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

dsengupta0628

I would leave CMakeLists.txt out from this set of change at least- if at all we want to take it, it should be in a separate PR.

maliberty · 2026-04-06T14:24:27Z

CMakeLists.txt

 option(USE_TCL_READLINE "Use TCL readline package" ON)
 option(ENABLE_TSAN "Compile with thread santizer enabled" OFF)
 option(ENABLE_ASAN "Compile with address santizer enabled" OFF)
+option(ALLOW_WARNINGS "Allow compiler warnings without failing the build" OFF)


In OR we make the default ON so that users compiling with various compilers won't get stuck. In the CI we set it OFF to catch development issues. Developers can do that same locally.

OK. I'll revert this CMakeLists.txt change. Let's revist it later with other considerations.

maliberty · 2026-04-06T14:27:32Z

sdc/test/sdc_exception_merge_priority.tcl

-# sta::remove_constraints
-# report_checks
-puts "remove_constraints: skipped (API removed)"
-
 # Re-add constraints for write_sdc


Just removing the comment doesn't really fix the issue. We are not re-adding anything as the prior constraints are still present. I don't think these tests test what they think they do.

You're right. I'll clean them up tomorrow.

jhkim-pii and others added 3 commits April 2, 2026 22:27

Merge branch 'fix-exception-override-priority' into secure-test-suite…

619d6bc

…-cleanup Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

Merge branch 'master' of https://github.com/The-OpenROAD-Project-priv…

7d0e0bb

…ate/OpenSTA into secure-test-suite-cleanup Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

openroad-ci assigned jhkim-pii Apr 3, 2026

jhkim-pii requested review from dsengupta0628 and maliberty April 3, 2026 04:38

maliberty previously approved these changes Apr 3, 2026

View reviewed changes

maliberty enabled auto-merge April 3, 2026 07:28

dsengupta0628 suggested changes Apr 3, 2026

View reviewed changes

dsengupta0628 reviewed Apr 3, 2026

View reviewed changes

jhkim-pii added 2 commits April 5, 2026 22:42

Merge remote-tracking branch 'origin/master' into secure-test-suite-c…

1170781

…leanup Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

build: Add -Werror by default

0c15e18

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

jhkim-pii requested a review from dsengupta0628 April 5, 2026 15:23

jhkim-pii and others added 4 commits April 6, 2026 14:35

Merge branch 'master' of https://github.com/The-OpenROAD-Project-priv…

d6a8044

…ate/OpenSTA into secure-test-suite-cleanup Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

jhkim-pii mentioned this pull request Apr 6, 2026

Remove dead remove_constraints placeholder blocks from SDC tests #338

Closed

dsengupta0628 suggested changes Apr 6, 2026

View reviewed changes

maliberty reviewed Apr 6, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up test suite: remove dead code, fix race conditions, centralize helpers#332

Clean up test suite: remove dead code, fix race conditions, centralize helpers#332
openroad-ci wants to merge 9 commits intoThe-OpenROAD-Project:masterfrom
The-OpenROAD-Project-staging:secure-test-suite-cleanup

openroad-ci commented Apr 3, 2026

Uh oh!

jhkim-pii commented Apr 3, 2026

Uh oh!

dsengupta0628 left a comment

Uh oh!

dsengupta0628 left a comment

Uh oh!

jhkim-pii commented Apr 3, 2026

Uh oh!

jhkim-pii commented Apr 5, 2026

Uh oh!

dsengupta0628 left a comment

Uh oh!

maliberty Apr 6, 2026

Uh oh!

jhkim-pii Apr 6, 2026

Uh oh!

maliberty Apr 6, 2026

Uh oh!

jhkim-pii Apr 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

openroad-ci commented Apr 3, 2026

Summary

Regression

Related

Uh oh!

jhkim-pii commented Apr 3, 2026

Uh oh!

dsengupta0628 left a comment

Choose a reason for hiding this comment

Uh oh!

dsengupta0628 left a comment

Choose a reason for hiding this comment

Uh oh!

jhkim-pii commented Apr 3, 2026

Uh oh!

jhkim-pii commented Apr 5, 2026

Uh oh!

dsengupta0628 left a comment

Choose a reason for hiding this comment

Uh oh!

maliberty Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

jhkim-pii Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

maliberty Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

jhkim-pii Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants