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

Vector Approx matcher #1499

Merged
merged 1 commit into from
Apr 20, 2019
Merged

Vector Approx matcher #1499

merged 1 commit into from
Apr 20, 2019

Conversation

williamjameshandley
Copy link
Contributor

Description

In addition to Equals and UnorderedEquals, this PR implements an Approx matcher for std::vector.

It is implemented along the lines of Equals and UnorderedEquals, with inspiration from previous attempts in issue #760, and this StackOverflow discussion

GitHub Issues

Addresses #760

@codecov
Copy link

codecov bot commented Jan 14, 2019

Codecov Report

Merging #1499 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1499   +/-   ##
=======================================
  Coverage   80.72%   80.72%           
=======================================
  Files         121      121           
  Lines        3424     3424           
=======================================
  Hits         2764     2764           
  Misses        660      660

@horenmar
Copy link
Member

Thanks, this looks good.

I have one note and one nitpick though.

Note: You should also add a test against strong typedef.
Nitpick: I think " is approx" would work much better as the stringification string inside the matcher.

@williamjameshandley
Copy link
Contributor Author

williamjameshandley commented Jan 25, 2019

Nitpick: I think " is approx" would work much better as the stringification string inside the matcher.

I'm fine with adjusting this.

Note: You should also add a test against strong typedef.

I'm not sure exactly what you mean by this. Could you give an example elsewhere in the code?

I also have a question -- currently I need to use Catch::Approx, otherwise it conflicts with Catch::Details::Aprox, and the compiler can't resolve the the ambiguity. What is the most consistent way to deal with this? I could change it to VectorApprox, but then that's not consistent with Equals or Contains

@horenmar
Copy link
Member

If you go to UsageTests/Approx.tests.cpp, on line 192 there is a test that tests Approx against a strong-typedef, that is a class that can be explicitly converted to double, but is not a double itself. It should work because of the underlying Approx usage, but it would be nice to have it documented with a test.


Thinking about this further, I think this actually opens up a philosophical question about the constructor of the Matcher. Specifically, whether the constructor shouldn't be templated and allow for vector filled with types that are convertible to double, but are not Ts themselves -- this would allow for e.g.

std::vector<Temperature> temps;
// fill temps somehow
REQUIRE_THAT(temps, Catch::Approx({17.2, 17.3, 18.2}));

Which is nicer that having to convert the doubles to temperatures by yourself.


As to your other question, I am afraid that is unfixable without a different name, because of the using Catch::Detail::Approx in catch.hpp that cannot be removed without breaking backwards compatibility.
You can also stop it from being ambigous by bringing in the function explicitly:

using Catch::Matchers::Approx;
CHECK_THAT(v3, Approx(v3));
CHECK_THAT(v4, Approx(v4));

works.

@williamjameshandley
Copy link
Contributor Author

You can also stop it from being ambigous by bringing in the function explicitly:

I've opted for this solution.

Regarding the strong typedefs, I had a go at this, but the main issue I seem to be getting if I try to implement a new std::initialiser_list call like so:

    template<typename T>                                                                                                                                                                          
    Vector::ApproxMatcher<T> Approx( std::initializer_list<T> const& comparator ) {                                                                                                               
        return Vector::ApproxMatcher<T>( comparator );                                                                                                                                            
    } 

is that this tends to cause segfaults due to the constant reference implicit in the std::vector<T> const& m_comparator; member function.

This also seems like a more wholesale change, given that it doesn't work for the Equals, Contains etc methods. Perhaps I could submit an issue to upgrade the whole include/internal/catch_matchers_vector.h header?

@horenmar horenmar merged commit 91b617c into catchorg:master Apr 20, 2019
@horenmar
Copy link
Member

I modified the tests a bit and merged it. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants