[WIP] Add namer that allows users full customization

[Corristo]

Description

This PR adds a namer can be used to completely cusomize where approved and received files will be stored. This allows to e.g. make the path dependend on the run time location of the test binary instead of the source location for cases where the machine running the tests might not even have the source files checked out, as mentioned in #128 .

The solution

The new namer allows the user to supply two function objects to compute the path of the received and approved file respectively, given the TestName object and the expected file extension. That way these function objects may or may not use the file location stored in the TestName object depending on the need of the user.

However, I'm not satisfied with this solution, as it doesn't compose with other existing namers. For example, there already is a namer to store approved and received files in different directories, but when using this new namer to use a custom location the user has to manually re-implement that in the function objects they provide to the namer. Moreover, with GoogleTest ApprovalTests.cpp will still try to find the source file and print an error if it cannot find it, even if the path isn't even used later on.

This is meant as a basis to develop other ideas and further discussions and expected to change a lot, which is why I didn't add documentation or tests for this yet.

[claremacrae]

Thank you very much for this.

Yes, the current Google test integration has some quite specific customisations:

https://github.com/approvals/ApprovalTests.cpp/blob/master/doc/UsingGoogleTests.md

I can imagine they might get under the feet of your new code...

[Corristo]

Yes, the current Google test integration has some quite specific customisations:

I considered adding yet another customization to the google test integration class to deactivate the equivalence check entirely, but it also doesn't seem very user friendly to make them set that flag in addition to using the new namer just to be able to run the tests on a machine that doesn't have the source files. It just seems redundant.

Alternatively I considered to move the equivalence check to ApprovalTestNamer::getOutputFileBaseName. That way all of these GoogleTest customizations would be available for other test frameworks, too, while at the same time ensuring that the source file name is needed only when a namer actually uses it. That would mean that a problem with the source files isn't reported immediately after starting the test binary, though, so I'm not sure if that is acceptable. I also didn't entirely think though the effects this might have on custom namers users have already written (i.e. how this affects compatibility).

[claremacrae]

Hi @Corristo I'm really sorry that we haven't picked this up yet... It's not forgotten.

[claremacrae]

I built this code, and tried sketching out a test, to see how to use the code...

Here's what I came up with:

#include "ApprovalTests/namers/CustomNamer.h"
#include "ApprovalTests/core/ApprovalNamer.h"
#include "ApprovalTests/namers/ApprovalTestNamer.h"
#include "doctest/doctest.h"

#include <iostream>
#include <memory>

using namespace ApprovalTests;

namespace
{
    std::string receivedNameFn(TestName namer, std::string extensionWithDot)
    {
        // Example outputs:
        // File name: /Users/.../ApprovalTests/ApprovalTests.cpp/tests/DocTest_Tests/namers/CustomNamerTests.cpp
        // Section:   CustomNamer
        // Extension: .received
        std::cout << "File name: " << namer.getFileName() << std::endl;
        for (const auto& section : namer.sections)
        {
            std::cout << "Section:   " << section << std::endl;
        }
        std::cout << "Extension: " << extensionWithDot << std::endl;
        return "received" + extensionWithDot;
    }

    std::string approvedNameFn(TestName /*namer*/, std::string extenstionWithDot)
    {
        return "approved" + extenstionWithDot;
    }
}

TEST_CASE("CustomNamer")
{
    CustomNamer namer(receivedNameFn, approvedNameFn);
    CHECK(namer.getReceivedFile(".received") == "received.received");
    CHECK(namer.getApprovedFile(".approved") == "approved.approved");
}

[Corristo]

Yeah, that is probably the simplest way to use it. I myself use it with a lambda and make use of the static member function useAsDefaultNamer as follows:

[[maybe_unused]] auto const use_custom_namer = 
  ApprovalTests::CustomNamer::useAsDefaultNamer(
     [this](ApprovalTests::TestName const& test_name, 
            std::string const& file_extension_with_dot) 
         -> std::string
     {
        return TestDataFolder() + "/received/" + test_name.sections.back()
          + file_extension_with_dot;
     },
     [this](ApprovalTests::TestName const& test_name, 
            std::string const& file_extension_with_dot)
         -> std::string
     {
         return TestDataFolder() + "/approved/" + test_name.sections.back()
           + file_extension_with_dot;
     });

Here TestDataFolder is a member function of the test fixture that returns a path relative to the test binary.

The way that the CustomNamer is currently implemented makes it very flexible, however I'm not quite happy with the design given the large amount of duplication in the "received" lambda and the "approved" lambda. I also dislike having to duplicate the code of the already existing SeparateApprovedAndReceivedDirectoriesNamer.

It'd probably be nicer to be able to customize the parts that make up the path separately, i.e. the base directory where approved and received files are stored (the folder where the test source file is located by default, TestDataFolder() in my case) and how the file name is computed from the TestName. The SeparateApprovedAndReceivedDirectoriesNamer could then become a decorator that takes off the filename with extension, adds the approved or received directory before finally appending the file name with extension again (possibly also removing any received/approved substrings of the file name).

I'm not sure this can be implemented in a backwards compatible manner, though, as the Namer interface might need to change. If backwards compatiblity isn't an issue I can go ahead and try to implement something along these lines. If you think the current CustomNamer is good enough then I'll add some tests and documentation.

[claremacrae]

Thanks - it was helpful to see your example code...

Ordinarily I would suggest that we get together for a chat - perhaps for a little while - to talk through ideas...

Unfortunately and unusually, for personal reasons, I am unlikely able to do that for 3 or 4 weeks...

So swapping ideas here for now is probably the best approach.

[claremacrae]

If backwards compatiblity isn't an issue I can go ahead and try to implement something along these lines.

I think it's relatively unlikely that many people have reimplemented Namer - and we have a pattern for making breaking changes over time.... so if you are willing to put more time in, then exploring your ideas - in code - would be really helpful.

And as you're going to the trouble of adding a new feature, it would be nice for it to be easy to use...

[claremacrae]

1. As a possible extra - or alternative - approach - I have wondered whether there might be value in exploring some kind of templating system.

We already have this:

https://github.com/approvals/ApprovalTests.cpp/blob/2d661d4d8e77442d57e796e7456135bdda9c427e/ApprovalTests/reporters/DiffInfo.cpp#L6-L20

These are used in defining where diff tools are located, and how to define their command line arguments...

The strings in curly braces get expanded at run time.

2. I was wondering if it would be possible - and worthwhile - to define the types of components that might go in to forming the parts of an approved filename and a received filename... So that someone might be able to construct a string to define where to put the files - e.g. off the top of my head:

{TestFolder}/{ReceivedOrApproved}/{RelativePathOfSourceDirectoryFromSourceRoot}/{SourceFileName}-{SectionNames}.{FileExtension}

Please don't take these names at all literally - I'm just trying to suggest possible ideas or concepts!!!

3. Another part of me thinks that this templating approach would need a bunch of thought and time to define it well... And is widening the goal posts way too far! So it might be better to divide the two concepts up, so:

   1. Introduce a nicely usable CustomNamer, based on what you have
   2. As a possible later step, introduce something like a TemplatingCustomNamer - that is implemented using a CustomNamer

What do you think @Corristo ?

[claremacrae]

Hi @Corristo - we're picking this up, and have been experimenting with what the docs might look like.

This is our current thinking:

https://github.com/approvals/ApprovalTests.cpp/blob/master/doc/how_tos/CreateCustomNamer.md

We'd love to hear what you think - nothing is cast in stone...

[Corristo]

If I understand the proposal correctly I think I prefer the old idea of having the templates where a user can place the parts they'd like to use wherever it fits best instead of having the fixed structure where one can only replace certain parts.

For example, right now the project I'm working on is using the following directory structure:

tests
  |----- feature1
  |   | ------- feature1_tests.cpp
  |   | ------- testdata
  |   |      |------ input1.txt
  |   |      |------ input2.txt
  |   |      |------- ....
  |   |      |------- approved
  |   |          |-------- test1.approved.txt
  |   |          |-------- test2.approved.txt
  |   |          |--------- ....
  |----- feature2
  |   | ------- feature2_tests.cpp
  |   | ------- testdata
  |   |      |------ input1.txt
  |   |      |------ input2.txt
  |   |      |------- ....
  |   |      |------- approved
  |   |          |-------- test1.approved.txt
  |   |          |-------- test2.approved.txt
  |   |          |--------- ....
  |----- .....

The tests for a particular feature all use the same test fixture, therefore the TestNames are of the form Feature1.test1, Feature1.test2, .... Due to this directory structure it doesn't add any value to have the name of the source file or fixture included in the file name of the approved file, which is why we don't add them in our custom namer.

If I understand the proposed solution correctly there would be no way to influence the actual file name, as the builder only allows to edit the test folder part, so the approved files would be named feature1.test1.approved.txt. I don't see a reason why we couldn't use the proposed solution, though, as the name of these files don't really matter all that much.

I see the appeal of this solution as it is generic enough to satisfy most needs, while also being much easier to implement than a templating system, in particluar if you'd want to allow dropping certain parts of the test name, e.g. by allowing to use {TestName[1]} to only use the test case name and not the name of the fixture like we do currently.

[isidore]

We believe that the current version of TemplatedCustomNamer should handle these cases well enough. And are going to close this. if you disagree, we should do a call and come to a better conclusion