-
Notifications
You must be signed in to change notification settings - Fork 91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor code duplications in dense_kernels tests #1423
Conversation
f2a6807
to
ed4c387
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is still too complex to review thoroughly. But I think it might be generally impossible to get this into an easy-to-review state due to the reordering. The only way I can think of is to split this into many more smaller PRs (probably 10 each with 200 LOC changes). But that is unreasonable in our workflow.
So I would be fine merging this, as long as the tests pass.
LGTM, reviewing the commits individually in VSCode makes it bearable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Only concern is that the tests that used stride=num_cols were updated to use non-standard stride instead, for example for ELL and Hybrid. But in general, it is probably okay as that is the more general case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests with DenseWithIndexType are in the middle of the test with Dense.
one more commit to reorder them again?
class Dense{};
TYPED_TEST(Dense, ...)
class DenseWithIndexType{};
TYPED_TEST(DenseWithIndexType)
class DenseComplex{};
TYPED_TEST(DenseComplex)
Also make the fixture close to the testing
Ignore this comment. I review the code in the reversed order
- Remove unused declarations - Consistent variable naming Co-authored-by: Marcel Koch <[email protected]>
ed4c387
to
4cc9692
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information The version of Java (11.0.3) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17. |
Pulled out of #1415, this refactors the dense reference tests to get instantiated for all index types automatically.