Repository navigation
Add unit test covering narrow multiplier area estimates - #517
samvratgowda wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## development #517 +/- ##
=============================================
+ Coverage 93.8% 93.8% +0.1%
=============================================
Files 30 30
Lines 7178 7178
=============================================
+ Hits 6729 6732 +3
+ Misses 449 446 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
fdxmw
left a comment
fdxmw
left a comment
There was a problem hiding this comment.
Thank you for improving PyRTL's test coverage!
| self.assertGreater(logic_area, 0) | ||
| areas.append(logic_area) | ||
|
|
||
| self.assertEqual(areas, sorted(areas)) |
There was a problem hiding this comment.
I don't think this really tests the special-case code for small multipliers? I think this checks that smaller-bitwidth multipliers have smaller areas, which should be true for all multipliers. If you removed the special-case code entirely, I think this test would still pass?
It seems better to directly compare each area result against a constant expected value, with assertAlmostEqual
| def test_area_est_small_multipliers(self): | ||
| """Multipliers narrower than four bits use their own area estimates.""" | ||
| areas = [] | ||
| for width in (1, 2, 3, 4): |
There was a problem hiding this comment.
See the comment on L111 first.
I think this is four tests masquerading as one test :) These kinds of tests are usually harder to debug, because when something goes wrong, you usually want to know what width was, but that does not show up in a stack trace.
You can work around this by moving the loop body into a new function, then writing four two-line tests that just call the new function. For example:
def test_area_est_bitwidth_1_multiplier(self):
area = self.small_multiplier_area(bitwidth=1)
self.assertAlmostEqual(area, ...)This way, when a failure occurs, it should be clear what the problematic width was.
While you're here, consider renaming width to bitwidth, which is a little more specific.
This branch has not been deployed
2 participants
No description provided.