bugfix: unused function complex_numbers_test.cpp

[Lucky-Armin]

in exercises/practice/complex_numbers/complex_numbers_test.cpp: when running the first few test cases without the use of the require_approx_equal helper function, a warning is issued which will be treated as an error

[github-actions]

Hello. Thanks for opening a PR on Exercism 🙂

We ask that all changes to Exercism are discussed on our Community Forum before being opened on GitHub. To enforce this, we automatically close all PRs that are submitted. That doesn't mean your PR is rejected but that we want the initial discussion about it to happen on our forum where a wide range of key contributors across the Exercism ecosystem can weigh in.

You can use this link to copy this into a new topic on the forum. If we decide the PR is appropriate, we'll reopen it and continue with it, so please don't delete your local branch.

If you're interested in learning more about this auto-responder, please read this blog post.

---

Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it.

[github-actions]

This PR touches files which potentially affect the outcome of the tests of an exercise. This will cause all students' solutions to affected exercises to be re-tested.

If this PR does not affect the result of the test (or, for example, adds an edge case that is not worth rerunning all tests for), please add the following to the merge-commit message which will stops student's tests from re-running. Please copy-paste to avoid typos.

[no important files changed]

For more information, refer to the documentation. If you are unsure whether to add the message or not, please ping @exercism/maintainers-admin in a comment. Thank you!

[vaeng]

Hm. Is that something new, that is flagged now, but wasn't before? @ahans

[vaeng]

Seems reasonable.

Where did the error occur for you? In the test runner?

[ahans]

It will probably happen whenever you have all tests commented out that don't use that function. The maybe_unused fixes it, but I think we have better options.

First, having a static function is not recommended in C++ for that usecase. Using an anonymous namespace would be better. Not sure it would not create a warning/error though.

Second, having functions for checks like that I consider a bit of an anti pattern. If there is a test failure, it will be reported for the line of the function and not the line in the test case that we are actually interested in. I would just skip this and use the two calls wherever they are needed. Alternatively, writing a proper Catch2 matcher would be the better option over such a plain function.

[Lucky-Armin]

It will probably happen whenever you have all tests commented out that don't use that function. The maybe_unused fixes it, but I think we have better options.

First, having a static function is not recommended in C++ for that usecase. Using an anonymous namespace would be better. Not sure it would not create a warning/error though.

Second, having functions for checks like that I consider a bit of an anti pattern. If there is a test failure, it will be reported for the line of the function and not the line in the test case that we are actually interested in. I would just skip this and use the two calls wherever they are needed. Alternatively, writing a proper Catch2 matcher would be the better option over such a plain function.

I opted for solution #2 and added a custom Catch2 matcher

[ahans]

Thanks a lot for taking the effort of implementing a proper matcher. Not only did this fix the compiler warning (treated as error in our setup), but also made this test better in following Catch2 best practices! I hope you don't mind that I did a few stylistic changes which I pushed to your branch before merging.

[blakelewis]

In case you haven't seen the discussion in the forum, this code currently fails to compile in the test runner. The code is written for catch2 v2, while the Dockerfile in the test runner gets the latest (v3) version and there are incompatible changes between v2 and v3. There seems to be a choice between changing the Dockerfile to get the older version or updating the test code to use v3.

[vaeng]

We would "just" update the catch version on this exercise of the client copy with v3. It's a "feature", that the client side is done on a per exercise basis.

It's mostly a cop paste job and the custom comparison has to be redone.. sorry for not seeing this, my headspace currently doesn't allow for exercise tasks to be run 😵

[ahans]

@vaeng I have this already fixed locally. Will open a PR in a moment. The only thing that needed fixing was the namespace. "Old-style matchers" are still supported.

What again was the reason we have a different Catch2 version in the runner than we have here next to the exercises and in test/catch.hpp?

[vaeng]

I'm not sure. Probably something to do with the ever elusive combination of "when we update from c++17, when we can have shared user files and laziness to update every single exercise"