[CI] Add tests to common_engine #5804
Conversation
|
Thanks for your contribution! |
kesmeey
changed the title
There was a problem hiding this comment.
Pull request overview
This PR adds 15 new test methods to the TestCommonEngine class to validate the existence and callability of various engine methods and attributes. However, the PR has critical issues including a misplaced if __name__ == "__main__" block that breaks the test file structure, and several tests with redundant assertion logic that provides no meaningful validation.
Key Changes:
- Added 8 tests checking method existence and callability (lines 191-229)
- Added 7 tests checking configuration and component attributes (lines 231-326)
- Introduced a duplicate
if __name__ == "__main__"block that causes structural issues (lines 852-853)
| for attr in dp_attrs: | ||
| if hasattr(dp, attr): | ||
| self.assertTrue(hasattr(dp, attr)) |
There was a problem hiding this comment.
The assertion logic here is redundant and provides no value. Inside the if hasattr(dp, attr) condition, you already know the attribute exists, so calling self.assertTrue(hasattr(dp, attr)) will always pass. This test should either directly assert on the attribute without the conditional check, or verify meaningful properties about the attribute values.
| for attr in sc_attrs: | ||
| if hasattr(sc, attr): | ||
| self.assertTrue(hasattr(sc, attr)) |
There was a problem hiding this comment.
The assertion logic here is redundant and provides no value. Inside the if hasattr(sc, attr) condition, you already know the attribute exists, so calling self.assertTrue(hasattr(sc, attr)) will always pass. This test should either directly assert on the attribute without the conditional check, or verify meaningful properties about the attribute values.
tests/engine/test_common_engine.py
Outdated
| if __name__ == "__main__": | ||
| unittest.main() | ||
|
|
There was a problem hiding this comment.
This if __name__ == "__main__" block is incorrectly placed in the middle of the TestCommonEngineAdditionalCoverage class. It should be removed from this location. The first occurrence at line 328 is correctly placed at the end of the TestCommonEngine class, and this duplicate causes all subsequent test methods (starting from line 855) to become unreachable dead code since they appear after the main block but are still indented as class methods.
| if __name__ == "__main__": | |
| unittest.main() |
| for attr in model_attrs: | ||
| if hasattr(model_cfg, attr): | ||
| self.assertTrue(hasattr(model_cfg, attr)) |
There was a problem hiding this comment.
The assertion logic here is redundant and provides no value. Inside the if hasattr(model_cfg, attr) condition, you already know the attribute exists, so calling self.assertTrue(hasattr(model_cfg, attr)) will always pass. This test should either directly assert on the attribute without the conditional check, or verify meaningful properties about the attribute values.
| for attr in cache_attrs: | ||
| if hasattr(cache_cfg, attr): | ||
| self.assertTrue(hasattr(cache_cfg, attr)) |
There was a problem hiding this comment.
The assertion logic here is redundant and provides no value. Inside the if hasattr(cache_cfg, attr) condition, you already know the attribute exists, so calling self.assertTrue(hasattr(cache_cfg, attr)) will always pass. This test should either directly assert on the attribute without the conditional check, or verify meaningful properties about the attribute values.
| for attr in parallel_attrs: | ||
| if hasattr(parallel_cfg, attr): | ||
| self.assertTrue(hasattr(parallel_cfg, attr)) |
There was a problem hiding this comment.
The assertion logic here is redundant and provides no value. Inside the if hasattr(parallel_cfg, attr) condition, you already know the attribute exists, so calling self.assertTrue(hasattr(parallel_cfg, attr)) will always pass. This test should either directly assert on the attribute without the conditional check, or verify meaningful properties about the attribute values.
| for attr in struct_attrs: | ||
| if hasattr(struct_cfg, attr): | ||
| self.assertTrue(hasattr(struct_cfg, attr)) |
There was a problem hiding this comment.
The assertion logic here is redundant and provides no value. Inside the if hasattr(struct_cfg, attr) condition, you already know the attribute exists, so calling self.assertTrue(hasattr(struct_cfg, attr)) will always pass. This test should either directly assert on the attribute without the conditional check, or verify meaningful properties about the attribute values.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5804 +/- ##
==========================================
Coverage ? 65.55%
==========================================
Files ? 337
Lines ? 43082
Branches ? 6639
==========================================
Hits ? 28243
Misses ? 12733
Partials ? 2106
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
kesmeey
changed the title
2 participants
Motivation
No need
Modifications
No need
Usage or Command
N/A
Accuracy Tests
N/A
Checklist
[FDConfig],[APIServer],[Engine],[Scheduler],[PD Disaggregation],[Executor],[Graph Optimization],[Speculative Decoding],[RL],[Models],[Quantization],[Loader],[OP],[KVCache],[DataProcessor],[BugFix],[Docs],[CI],[Optimization],[Feature],[Benchmark],[Others],[XPU],[HPU],[GCU],[DCU],[Iluvatar],[Metax]]pre-commitbefore commit.releasebranch, make sure the PR has been submitted to thedevelopbranch, then cherry-pick it to thereleasebranch with the[Cherry-Pick]PR tag.