Skip to content
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

kokkos: refactor duplicated test helpers #187

Merged
merged 10 commits into from
Apr 25, 2022

Conversation

mzuzek
Copy link

@mzuzek mzuzek commented Apr 22, 2022

This PR moves common Kokkos test utilities to shared headers - refactoring massive duplication and significantly shortening test files.

#define FOR_ALL_BLAS2_TYPES(TEST_DEF) \
TEST_DEF(double) \
TEST_DEF(float) \
TEST_DEF(complex_double)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awww, no complex_float?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is my fault because I started all tests using double, float and complex<double>, so Miko was following my setup :)
I created a new issue: #190 to add complex<float> later

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MikolajZuzek my preference would be to remove this macro. It is just 3 lines of code :) it does not save you much typing. Also, I think it adds another layer that one would have to find when looking at the tests.
I think this gtest_fixtures should only contain the fixtures. If you really want this macro, maybe we can put it in another file called test_macros.hpp or similar so that it is clear the distinction.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Macro removed on 6be6990. It seemed like a good idea couple gtest_fixtures.hpp types in a macro and place it close by, but maybe we can just remember to keep that synchronized in test files.

tests/kokkos-based/helpers.hpp Outdated Show resolved Hide resolved
tests/kokkos-based/helpers.hpp Outdated Show resolved Hide resolved
tests/kokkos-based/helpers.hpp Outdated Show resolved Hide resolved
tests/kokkos-based/helpers.hpp Outdated Show resolved Hide resolved
tests/kokkos-based/helpers.hpp Outdated Show resolved Hide resolved
@mzuzek mzuzek force-pushed the refactor-duplicated-test-helpers branch from 950bd4a to 51f0de4 Compare April 25, 2022 14:40
@mzuzek mzuzek force-pushed the refactor-duplicated-test-helpers branch from 51f0de4 to 46c2fec Compare April 25, 2022 14:44
Copy link
Member

@crtrott crtrott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly a good improvement. But where is this _blas2_signed_fixture thing coming from? Is that some semi pulic non-namespaced thing in our headers?

@crtrott crtrott merged commit a4d0607 into kokkos:main Apr 25, 2022
@mzuzek mzuzek deleted the refactor-duplicated-test-helpers branch April 30, 2022 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants