test: refactor upf_aes to use OpenDB assertions instead of DEF diff - #11152
test: refactor upf_aes to use OpenDB assertions instead of DEF diff#11152oharboe wants to merge 6 commits into
Conversation
![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 refactors the upf_aes regression test to directly query and validate region boundaries from the database block instead of writing and diffing external region files. The feedback suggests ensuring that regions have at least one boundary to prevent silent test failures, using braced expr with integer division to avoid floating-point precision issues, and using the eq operator for string comparisons in Tcl.
|
/gemini review |
|
Addressed the nit-picks in the latest commit: used |
There was a problem hiding this comment.
Code Review
This pull request replaces file-based diffing of DEF regions in test/upf_aes.tcl with programmatic verification using the database API. The feedback suggests comparing coordinates directly in DBUs instead of using integer division to avoid truncation masking small coordinate mismatches.
|
/gemini review |
|
Good catch! I've updated the comparison to check directly in DBUs to prevent truncation masking. The linter checks and tests passed locally, and I've force pushed the update. |
There was a problem hiding this comment.
Code Review
This pull request refactors the upf_aes regression test to programmatically verify region boundaries using OpenDB APIs instead of performing a file-based diff on the exported DEF file. The helper procedure write_regions_section and the reference .regions file have been removed. Feedback on the changes points out a critical bug where getBoundaries is called on the dbRegion object; this method does not exist in OpenDB and should be replaced with getBoxes to avoid a runtime error.
Refactors the upf_aes test to use idiomatic OpenDB Tcl API assertions, eliminating brittle DEF string-parsing and .ok file diffs. Historical Context & Why We Did This: Historically, this integration test placed the design and diffed the entire output DEF against a golden .defok file. When Bazel support was introduced (commit 2ea994e), it was discovered that the placement coordinates were numerically unstable between Bazel and CMake toolchains. To unblock the Bazel build, a workaround was added: extracting *only* the REGIONS block from the DEF via a custom Tcl text-parsing script, diffing it against a regionsok file, and disabling standard log checks. We are replacing this hack with the idiomatic approach: querying the OpenDB API directly in Tcl to verify that the UPF constraints correctly translated into database Regions. What We Considered But Didn't Do: 1. Writing a C++ Test: upf_aes is a full integration test (UPF parsing, floorplanning, global & detailed placement). Replicating this flow in C++ just to test region emission is verbose and goes against the project's paradigm of using Tcl for flow testing. Testing DEF parsing/serialization of regions in isolation is the responsibility of src/odb unit tests. 2. DEF Round-Trip Validation: We considered dumping the DEF, clearing the database, reading the DEF back in, and running our Tcl assertions to honor the original intent of testing DEF serialization. We rejected this because testing DEF serialization is not the responsibility of a UPF flow test, and it adds unnecessary complexity. Fixes: - Removes test/upf_aes.defok and test/upf_aes.regionsok - Removes Bazel-specific exceptions in test/BUILD for upf_aes Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
Replaced getBoundaries with getBoxes. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the upf_aes regression test to verify region boundaries directly via the database API instead of writing them to a file and performing a file diff. This removes the need for Bazel-specific exceptions and a separate regions file. Feedback on these changes suggests removing upf_aes from the check_log exclusion list in test/BUILD to ensure its log is verified, and tightening the boundary check in test/upf_aes.tcl to assert that each region has exactly one boundary box.
| [ | ||
| regression_test( | ||
| name = test_name, |
There was a problem hiding this comment.
The PR description mentions that Bazel-specific exceptions for upf_aes are removed. However, "upf_aes" is still present in the check_log exclusion list on line 188 (which is just below the current diff hunk):
check_log = False if test_name in PASSFAIL_TESTS + BIG_TESTS + ["upf_aes"] else True,Since the log output is now stable across Bazel and CMake, please remove "upf_aes" from that list so that the log file is verified during Bazel regression testing.
| set boxes [$region getBoxes] | ||
|
|
||
| if { [llength $boxes] == 0 } { | ||
| utl::error "UPF" 5 "Region $name has no boundaries" | ||
| } |
There was a problem hiding this comment.
Instead of only checking if the region has 0 boundaries, it is safer to assert that it has exactly 1 boundary box. This prevents cases where a region might unexpectedly have multiple duplicate or extra boundary boxes in the database.
set boxes [$region getBoxes]
if { [llength $boxes] != 1 } {
utl::error "UPF" 5 "Region $name expected 1 box, found [llength $boxes]"
}
|
/gemini review |
|
Reverted |
There was a problem hiding this comment.
Code Review
This pull request refactors the upf_aes test to perform programmatic database assertions on regions and their boundaries instead of relying on a file-based diff of the DEF output. It removes the helper procedure for writing regions and the corresponding reference file. The review feedback suggests a robust improvement to the test assertions by tracking and verifying the exact set of expected region names, preventing potential false positives where duplicate regions of the same name could bypass the current checks.
| set block [ord::get_db_block] | ||
| set regions [$block getRegions] | ||
|
|
||
| if { [llength $regions] != 2 } { | ||
| utl::error "UPF" 1 "Expected 2 regions, found [llength $regions]" | ||
| } | ||
|
|
||
| set dbu [$block getDefUnits] | ||
| foreach region $regions { | ||
| set name [$region getName] | ||
| set boundaries [$region getBoundaries] | ||
|
|
||
| if { [llength $boundaries] != 1 } { | ||
| utl::error "UPF" 5 "Region $name expected 1 box, found [llength $boundaries]" | ||
| } | ||
|
|
||
| foreach box $boundaries { | ||
| set xmin [$box xMin] | ||
| set ymin [$box yMin] | ||
| set xmax [$box xMax] | ||
| set ymax [$box yMax] | ||
|
|
||
| if { $name eq "PD_AES_1" } { | ||
| if { | ||
| $xmin != 30 * $dbu || $ymin != 30 * $dbu | ||
| || $xmax != 650 * $dbu || $ymax != 490 * $dbu | ||
| } { | ||
| utl::error "UPF" 2 "Region PD_AES_1 boundary mismatch (in DBU): $xmin $ymin $xmax $ymax" | ||
| } | ||
| } elseif { $name eq "PD_AES_2" } { | ||
| if { | ||
| $xmin != 30 * $dbu || $ymin != 510 * $dbu | ||
| || $xmax != 650 * $dbu || $ymax != 970 * $dbu | ||
| } { | ||
| utl::error "UPF" 3 "Region PD_AES_2 boundary mismatch (in DBU): $xmin $ymin $xmax $ymax" | ||
| } | ||
| } else { | ||
| utl::error "UPF" 4 "Unknown region $name" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The current assertion checks that there are exactly 2 regions and that any region named PD_AES_1 or PD_AES_2 has the correct boundaries. However, if there is a bug where one region is duplicated (e.g., two PD_AES_1 regions) and the other is missing, the test would still pass because the length is 2 and both regions match the PD_AES_1 boundary check.
To make the test more robust, we should track the names of the regions we have seen and assert at the end that both PD_AES_1 and PD_AES_2 were found.
set block [ord::get_db_block]
set regions [$block getRegions]
if { [llength $regions] != 2 } {
utl::error "UPF" 1 "Expected 2 regions, found [llength $regions]"
}
set seen_regions {}
set dbu [$block getDefUnits]
foreach region $regions {
set name [$region getName]
lappend seen_regions $name
set boundaries [$region getBoundaries]
if { [llength $boundaries] != 1 } {
utl::error "UPF" 5 "Region $name expected 1 box, found [llength $boundaries]"
}
foreach box $boundaries {
set xmin [$box xMin]
set ymin [$box yMin]
set xmax [$box xMax]
set ymax [$box yMax]
if { $name eq "PD_AES_1" } {
if {
$xmin != 30 * $dbu || $ymin != 30 * $dbu
|| $xmax != 650 * $dbu || $ymax != 490 * $dbu
} {
utl::error "UPF" 2 "Region PD_AES_1 boundary mismatch (in DBU): $xmin $ymin $xmax $ymax"
}
} elseif { $name eq "PD_AES_2" } {
if {
$xmin != 30 * $dbu || $ymin != 510 * $dbu
|| $xmax != 650 * $dbu || $ymax != 970 * $dbu
} {
utl::error "UPF" 3 "Region PD_AES_2 boundary mismatch (in DBU): $xmin $ymin $xmax $ymax"
}
} else {
utl::error "UPF" 4 "Unknown region $name"
}
}
}
if { [lsort $seen_regions] ne {PD_AES_1 PD_AES_2} } {
utl::error "UPF" 6 "Expected regions PD_AES_1 and PD_AES_2, found: $seen_regions"
}
|
Tracked expected regions explicitly and verify exactly one occurrence of each to prevent duplicates bypassing the checks. The tests are green. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the upf_aes regression test. Instead of writing a DEF file and diffing the regions section against a golden file, the test now directly queries the database block for regions and asserts their boundaries and coordinates in DBUs. The review feedback suggests simplifying the boundary check by directly extracting the single box using lindex instead of a nested foreach loop, as the code already asserts that there is exactly one boundary box.
| foreach box $boundaries { | ||
| set xmin [$box xMin] | ||
| set ymin [$box yMin] | ||
| set xmax [$box xMax] | ||
| set ymax [$box yMax] | ||
|
|
||
| if { $name eq "PD_AES_1" } { | ||
| if { | ||
| $xmin != 30 * $dbu || $ymin != 30 * $dbu | ||
| || $xmax != 650 * $dbu || $ymax != 490 * $dbu | ||
| } { | ||
| utl::error "UPF" 2 "Region PD_AES_1 boundary mismatch (in DBU): $xmin $ymin $xmax $ymax" | ||
| } | ||
| } elseif { $name eq "PD_AES_2" } { | ||
| if { | ||
| $xmin != 30 * $dbu || $ymin != 510 * $dbu | ||
| || $xmax != 650 * $dbu || $ymax != 970 * $dbu | ||
| } { | ||
| utl::error "UPF" 3 "Region PD_AES_2 boundary mismatch (in DBU): $xmin $ymin $xmax $ymax" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Since we have already asserted that $boundaries contains exactly one box (lines 53-55), we can directly extract the single box using lindex instead of using a nested foreach loop. This simplifies the code, reduces nesting, and improves readability.
set box [lindex $boundaries 0]
set xmin [$box xMin]
set ymin [$box yMin]
set xmax [$box xMax]
set ymax [$box yMax]
if { $name eq "PD_AES_1" } {
if {
$xmin != 30 * $dbu || $ymin != 30 * $dbu
|| $xmax != 650 * $dbu || $ymax != 490 * $dbu
} {
utl::error "UPF" 2 "Region PD_AES_1 boundary mismatch (in DBU): $xmin $ymin $xmax $ymax"
}
} elseif { $name eq "PD_AES_2" } {
if {
$xmin != 30 * $dbu || $ymin != 510 * $dbu
|| $xmax != 650 * $dbu || $ymax != 970 * $dbu
} {
utl::error "UPF" 3 "Region PD_AES_2 boundary mismatch (in DBU): $xmin $ymin $xmax $ymax"
}
}
|
Used |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the upf_aes regression test by replacing the file-diff-based region verification with direct database queries and assertions on region counts, names, and boundaries. The helper function write_regions_section and the golden .regionsok file have been removed, and log checking has been enabled for the test. Feedback on these changes suggests updating the test/upf_aes.ok file to prevent CI failures from the new log output, adding assertions to verify that the region type is FENCE to maintain test coverage, and parenthesizing multiplication operations in boundary comparisons for better readability.
| name = test_name, | ||
| size = "enormous" if test_name in BIG_TESTS else "medium", | ||
| check_log = False if test_name in PASSFAIL_TESTS + BIG_TESTS + ["upf_aes"] else True, | ||
| check_log = False if test_name in PASSFAIL_TESTS + BIG_TESTS else True, |
There was a problem hiding this comment.
Since check_log is now enabled (True) for upf_aes, the regression test framework will compare the test's log output against test/upf_aes.ok. Because the Tcl script was refactored to remove write_def and diff_file (and add new assertions), the log output has changed. Please ensure that test/upf_aes.ok is updated or created to match the new log output, otherwise the CI test will fail due to a log mismatch.
| set name [$region getName] | ||
| set boundaries [$region getBoundaries] |
There was a problem hiding this comment.
The original upf_aes.regionsok file asserted that both regions have TYPE FENCE. The new assertions verify the names and boundaries, but do not check the region type. To maintain full test coverage and prevent regressions where a region's type might be incorrectly assigned, consider adding an assertion to verify that the region type is indeed FENCE.
set name [$region getName]
set type [$region getRegionType]
if { $type ne "FENCE" } {
utl::error "UPF" 7 "Region $name expected type FENCE, found $type"
}
set boundaries [$region getBoundaries]
| if { $name eq "PD_AES_1" } { | ||
| if { | ||
| $xmin != 30 * $dbu || $ymin != 30 * $dbu | ||
| || $xmax != 650 * $dbu || $ymax != 490 * $dbu | ||
| } { | ||
| utl::error "UPF" 2 "Region PD_AES_1 boundary mismatch (in DBU): $xmin $ymin $xmax $ymax" | ||
| } | ||
| } elseif { $name eq "PD_AES_2" } { | ||
| if { | ||
| $xmin != 30 * $dbu || $ymin != 510 * $dbu | ||
| || $xmax != 650 * $dbu || $ymax != 970 * $dbu | ||
| } { | ||
| utl::error "UPF" 3 "Region PD_AES_2 boundary mismatch (in DBU): $xmin $ymin $xmax $ymax" | ||
| } | ||
| } |
There was a problem hiding this comment.
To improve readability and make the operator precedence explicit, consider parenthesizing the multiplication operations (30 * $dbu, etc.) in the boundary comparison expressions.
if { $name eq "PD_AES_1" } {
if {
$xmin != (30 * $dbu) || $ymin != (30 * $dbu)
|| $xmax != (650 * $dbu) || $ymax != (490 * $dbu)
} {
utl::error "UPF" 2 "Region PD_AES_1 boundary mismatch (in DBU): $xmin $ymin $xmax $ymax"
}
} elseif { $name eq "PD_AES_2" } {
if {
$xmin != (30 * $dbu) || $ymin != (510 * $dbu)
|| $xmax != (650 * $dbu) || $ymax != (970 * $dbu)
} {
utl::error "UPF" 3 "Region PD_AES_2 boundary mismatch (in DBU): $xmin $ymin $xmax $ymax"
}
}
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
Updated the check for the region type: replaced |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the upf_aes regression test to programmatically validate region boundaries and types in Tcl instead of performing a file diff against a golden .regionsok file. This includes removing the write_regions_section helper, deleting the static regions definition file, and updating the Bazel build configuration to enable standard log checking for upf_aes. There are no review comments, and I have no feedback to provide.
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
Curious why you closed this |
|
Too much going on. It snowed down in gemini nits. Feel free to open, I think it is done, just needs pushing across the finish line. Used to work before .tcl tinkering with nits. |
|
This test caused snags in CI... |
2 participants
Refactors the upf_aes test to use idiomatic OpenDB Tcl API assertions, eliminating brittle DEF string-parsing and .ok file diffs.
Historical Context & Why We Did This:
Historically, this integration test placed the design and diffed the entire output DEF against a golden .defok file. When Bazel support was introduced (commit 2ea994e), it was discovered that the placement coordinates were numerically unstable between Bazel and CMake toolchains. To unblock the Bazel build, a workaround was added: extracting only the REGIONS block from the DEF via a custom Tcl text-parsing script, diffing it against a regionsok file, and disabling standard log checks.
We are replacing this hack with the idiomatic approach: querying the OpenDB API directly in Tcl to verify that the UPF constraints correctly translated into database Regions.
What We Considered But Didn't Do:
Fixes: