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

Rename KRATOS_EXPECT_MATRIX_EQUAL to KRATOS_EXPECT_MATRIX_EQ #11899

Merged
merged 1 commit into from
Dec 19, 2023

Conversation

avdg81
Copy link
Contributor

@avdg81 avdg81 commented Dec 19, 2023

📝 Description
This PR extracts the changes related to the macro renaming from PR #11506. The aim is to assist in breaking down PR #11506 into smaller pieces that can be reviewed more easily and which can be merged back separately. This commit renames the macro and it updates all of its usages.

This commit extracts the changes related to the macro renaming from PR
#11506. The aim is to assist in breaking down PR #11506 into smaller
pieces that can be reviewed more easily and which can be merged back
separately. This commit renames the macro and it updates all of its
usages.
@avdg81 avdg81 self-assigned this Dec 19, 2023
@avdg81 avdg81 requested review from a team as code owners December 19, 2023 12:43
@avdg81
Copy link
Contributor Author

avdg81 commented Dec 19, 2023

@roigcarlo: May I kindly ask you to review this small PR?

@loumalouomega: Are these simple changes in line with one of your suggestions for PR #11506?

@loumalouomega
Copy link
Member

I am okay with changes

Copy link
Member

@roigcarlo roigcarlo left a comment

Choose a reason for hiding this comment

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

👍

@philbucher
Copy link
Member

does GTest have a matrix comparison?
Does it behave the same as ours?

@avdg81 avdg81 merged commit 54ae0fb into master Dec 19, 2023
17 checks passed
@avdg81 avdg81 deleted the geo/rename-expect-macro branch December 19, 2023 14:51
@philbucher
Copy link
Member

does GTest have a matrix comparison? Does it behave the same as ours?

ping @avdg81

@avdg81
Copy link
Contributor Author

avdg81 commented Dec 19, 2023

does GTest have a matrix comparison? Does it behave the same as ours?

ping @avdg81

@philbucher: As far as I know, GTest doesn't have an EXPECT and/or ASSERT macro that is targeted at matrices. However, I would expect that we can write a validation function (or macro) for this purpose that is composed of the elementary assertions that GTest offers. Alternatively, I can imagine that we write a matcher for this purpose that we then use in conjunction with the EXPECT_THAT macro. Does this answer your question?

@roigcarlo
Copy link
Member

roigcarlo commented Dec 20, 2023

Adding to @avdg81 answer, it does not have a direct matrix comparison, but it can compare any container that is iterable. The problem is that our matrices are not iterable (or at least it does not like the type of the iterator).

For the vectors its already done with iterators:

EXPECT_THAT(a, Pointwise(DoubleNear(tolerance), b));

Edit: Also, this works for our _near comparisons, but there is no such comparator for _relavie_near. We should create one for those, but I think this can be done once all the tests are working again under GTest

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