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

F!! [Boost].UT test framework integration support #58

Merged
merged 12 commits into from
Dec 20, 2019
Merged

F!! [Boost].UT test framework integration support #58

merged 12 commits into from
Dec 20, 2019

Conversation

lp55
Copy link

@lp55 lp55 commented Dec 13, 2019

Hi,

This is the initial implementation of the [Boost].UT test framework integration support, developed alongside @claremacrae
I see that I still have to build the tests for the approval tests of the integration with the test framework :) But please take a look to see if anything else is missing and if there are changes needed for what's been done.

@lp55 lp55 requested a review from isidore as a code owner December 13, 2019 00:32
ApprovalTests/integrations/ut/UTApprovals.h Show resolved Hide resolved
ApprovalTests/namers/ApprovalTestNamer.h Show resolved Hide resolved
doc/UsingUT.md Show resolved Hide resolved
doc/UsingUT.md Outdated Show resolved Hide resolved
doc/UsingUT.md Show resolved Hide resolved
ApprovalTests/integrations/ut/UTApprovals.h Show resolved Hide resolved
tests/UT_Tests/main.cpp Outdated Show resolved Hide resolved
@claremacrae
Copy link
Collaborator

This is great work - thank you very much indeed...

Some extra comments apart from the code review:

  • This is definitely on the right track, and will be really useful.
  • Thanks for using the F!! prefix!
  • What's the behaviour now when an Approval Test verify fails? Is the text of the exception written out? If not, can this be made to happen please, so users will know how to deal with test failures.
  • For writing tests of the naming, this one might give you some ideas: tests/DocTest_Tests/namers/DocTestNamerTests.cpp
  • At some point it would be good to have a ut release header in third_party - note the dance that we do with adding the release file with its version name, and then a one-line file that wraps around - see third_party/doctest/doctest.2.3.5.h for an example...

Thanks again!

Clare

@lp55
Copy link
Author

lp55 commented Dec 13, 2019

One thing, I caught a bug when compiling UT inside ApprovalTests. It was due to a min macro MSVC adds. I made a pull request on UT: boost-ext/ut#235
The developer seems very responsive, so this should be patched soon, and I'll up the minimum version for UT on the documentation afterwards.

@lp55
Copy link
Author

lp55 commented Dec 13, 2019

As for adding UT to the third_party, I recommend waiting for it to stabilize a bit more first. It's fairly new and still being heavily developed. You would have an obsolete version very quickly.

@claremacrae
Copy link
Collaborator

As for adding UT to the third_party, I recommend waiting for it to stabilize a bit more first. It's fairly new and still being heavily developed. You would have an obsolete version very quickly.

I understand...

I should have explained why... As we maintain Approval Tests, we would like to know that we haven't broken any of our integrations, which means we need to run all our tests, including those of all our integrations, in our CI...

In the longer run, I know that we could improve our CMake files a bit, and download dependencies - but for now, the way we do this is via third_party...

So having an out-of-date file in third_party is preferable to not running some of our tests...

@claremacrae
Copy link
Collaborator

Luiz, I should have said, I'm really happy to pair again on any of the remaining bits of this. If that would help, let's set it up by email...

@lp55
Copy link
Author

lp55 commented Dec 13, 2019

As for the behaviour when an Approval Test verify fails, the user can just do something like this:

expect(nothrow([] { 
    auto section = NamerFactory::appendToOutputFilename("test 1");
    Approvals::verify("Approval Tests can verify text via the golder master method"); 
})) << "Approval test error";

I can update the docs with that. Or do you have something else in mind?

@lp55
Copy link
Author

lp55 commented Dec 19, 2019

I've added more tests. Tomorrow I'll improve the documentation and it should be ok to merge.

@lp55
Copy link
Author

lp55 commented Dec 20, 2019

Hi, I've fixed a check on cmakefiles to only compile ut integration with the supported compilers and hand fixed the UsingUT.md documentation (I don't have the software to generate the mds from the source). I fixed the snippets to use from real code now. Should be good to merge. If there's anything else needed to be done to be ready to merge, just ask.

@claremacrae
Copy link
Collaborator

The tests all pass for me, so I'm accepting this - thanks very much @lp55!

Afterwards, I'll pull in the latest ut header, with exception and source_location fixes - and then remove the try/catch block from the tests..

@claremacrae claremacrae merged commit 1f735b2 into approvals:master Dec 20, 2019
@claremacrae claremacrae added this to the v.7.0.0 release milestone Dec 20, 2019
@claremacrae
Copy link
Collaborator

Hi @lp55 - it's all looking good!

I added a "Minimum C++ Version" column to this table: https://github.com/approvals/ApprovalTests.cpp/blob/master/doc/GettingStarted.md#choosing-a-testing-framework

I said C++17 for UT, but then I noticed it now says CMAKE_CXX_STANDARD 20 in the CMakeLists.txt for the tests...

Should I change the table to say 20, or can the CMake file be changed back to 17?

Thanks!

@lp55
Copy link
Author

lp55 commented Dec 20, 2019

Hi @claremacrae,

Thanks for accepting the pull request!
Actually is better to say C++20, because source_location is actually a C++20 feature, and for ApprovalTest it's required, event if the UT framework itself can work with C++17.

@claremacrae
Copy link
Collaborator

Thanks @lp55 - I've updated the table to C++20 and added a footnote explaining why:

https://github.com/approvals/ApprovalTests.cpp/blob/master/doc/GettingStarted.md#choosing-a-testing-framework

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

Successfully merging this pull request may close these issues.

2 participants