Provide a way to select the Reporter at run-time

[claremacrae]

I’d really like to remove the need to edit and recompile to change the reporter used, probably by providing a command-line argument to do so.

We could base the option names on the reporter classes, with the Reporter suffix removed.

Examples that spring to mind:

-r BeyondCompareReporter
--reporter Quiet
-r Clipboard
-r AutoApprove
-r ‘C:\path to my program\...exe’
-r ‘mydifftool -1 {received} -2 {approved} &’
-r ‘someconsolediff {received} {approved}’

We would want to control whether the tool runs in the foreground or background, if it launches another process.

[claremacrae]

We could use one of these for implementation, if we could figure out how to include them in the single header without conflict for users, and with Catch2 which uses the first one too:

• https://github.com/catchorg/Clara
• https://github.com/bfgroup/Lyra

[claremacrae]

The Python implementation uses

pytest --approvaltests-use-reporter='PythonNative'

https://github.com/approvals/ApprovalTests.Python

[alepez]

What about an environmental variable? It would be simpler to implement and does not need new dependencies (we already have safeGetEnv in SystemUtils). The env var may be something like APPROVALTESTS_USE_REPORTER=Clipboard

And if in the future we want to add the opportunity to use cli arguments instead, the two methods can coexist (cli arguments override env var)

[claremacrae]

What about an environmental variable? It would be simpler to implement and does not need new dependencies (we already have safeGetEnv in SystemUtils). The env var may be something like APPROVALTESTS_USE_REPORTER=Clipboard

And if in the future we want to add the opportunity to use cli arguments instead, the two methods can coexist (cli arguments override env var)

Awesome idea - thank you!!!

[alepez]

And letting users select a different tool at runtime can be a workaround for known issues with clion #138

[claremacrae]

And letting users select a different tool at runtime can be a workaround for known issues with clion #138

I had a feeling you might be thinking of that!!!

Would you like to have a go at pairing on it?

[alepez]

Would you like to have a go at pairing on it?

Sure! Next week. I'll write you privately to schedule it.

[claremacrae]

Sketch of ideas discussed, so we don't lose it:

    APPROVAL_TESTS_REGISTER_REPORTER(ClipboardReporter);
    // auto reporter = ReporterFactory::reporter("ClipboardReporter");
    // auto reporter = ReporterRegistry::reporter("ClipboardReporter");

    // Static initialisation order fiasco
    //
}

// static initalisation

// 20 .cpp static init
    // factory

class ReporterFactory
{
    void registerReporter(name, reporter)
    {
        registry()->
    }
    std::map<std::string, std::shared_ptr<Reporter>>& registry()
    {
        static std::map<std::string, std::shared_ptr<Reporter>> registry;
    }
}

[claremacrae]

All that remains for this now is to write the documentation for the initial implementation and release it...

[claremacrae]

More thoughts...

1. Maybe add some console output if the user sets the environment variable contains a value not recognised.

2. We tested this in DocTest_Tests and it worked...

Then I remembered that the only reason it worked is because we had previously added this code to the main:

auto defaultReporterDisposer =
    Approvals::useAsDefaultReporter(std::make_shared<EnvironmentVariableReporter>());

3. It's still unresolved whether to add support for this behaviour by default.

[alepez]

Oh! I didn't notice it was added here

If we make it enabled by default, it's not a breaking change, because the behavior when the env var is not defined is the same as before, right? So it may be added to DiffReporter (which is the default):

In file ApprovalTests/reporters/DiffReporter.cpp

namespace ApprovalTests
{
    DiffReporter::DiffReporter()
        : FirstWorkingReporter({
                                new EnvironmentVariableReporter(),
                                new Mac::MacDiffReporter(),
                                new Linux::LinuxDiffReporter(),
                                new Windows::WindowsDiffReporter()})
    {
    }
}

[alepez]

Note that now we can even select the right reporter for the current operating system. So, instead of trying MacDiffReporter first on Linux, we shouldgive priority to LinuxDiffReporter.

I made some experiments here some weeks ago.

But this should be another issue.

[claremacrae]

I experimented with exactly the same change as you suggested in DiffReporter above...

Right now, EnvironmentVariableReporter uses a DiffReporter as a fall-back option, which I think would cause an infinite loop... Changing EnvironmentVariableReporter to just return false on failure/no env var set works fine.

If we were to use EnvironmentVariableReporter on every call, I would suggest changing our new factory implementation so that it didn't create a new map of all the reporters on every call, as that's just wasted work - but that's a trivial change.

Let's keep experimenting with this, and see what it looks like in practice...

[alepez]

Right, now the ReporterFactory in EnvironmentVariableReporter is created each time report is called, while it should be a member of EnvironmentVariableReporter.

I think EnvironmentVariableReporter should return false on failure, so it has the same behavior as any other reporter, letting it compose well with external fallbacks systems (like FirstWorkingReporter)

[alepez]

Given that, if users want to say "use EnvironmentVariableReporter or fall back to my favourite reporter" they can just set the default reporter to FirstWorkingReporter({ new EnvironmentVariableReporter(), new MyFavouriteReporter() }) and that's all.

[claremacrae]

Thanks - We're thinking along the same lines...

This is a great idea - I hadn't thought of that:

Given that, if users want to say "use EnvironmentVariableReporter or fall back to my favourite reporter" they can just set the default reporter to FirstWorkingReporter({ new EnvironmentVariableReporter(), new MyFavouriteReporter() }) and that's all.

[claremacrae]

One scenario where this would be useful is where a team of several people work together on the same code, and they have different preferences as to which reporter to use, or maybe not all of them have licences to the same diff tool.

[claremacrae]

Remaining steps:

Code

• [x] Add EnvironmentVariableReporter at start of DiffReporter constructor
• [x] Remove line containing EnvironmentVariableReporter from tests/DocTest_Tests/main.cpp
• [x] Remove the repetitious construction of the map in ReporterFactory::createReporter
• [x] Remove repetition of ReporterFactory creation in EnvironmentVariableReporter
• [x] Handle failure if the environment variable value is not recognised...
  • [x] Hard failure if env variable set and reporter not found
  • [x] And if the selected reporter is not available
  • [x] help message like "this value not found from this possible range of values..." - that is, provide a Help message, showing what original unmodified values it supports - and link to online docs
• [x] Trim whitespace off both ends of env var values before search
• [ ] Add to release notes and Features page

Docs

• [x] Document use of setting environment variable for EnvironmentVariableReporter
• [x] Update docs for adding a new reporter, to say to add them to the reporter factory
• [x] DON'T - Add new page to list of all customisation points - no action required
• [x] Add new page to any lists of pages relevant to reporters
• [x] DON'T - Document use of EnvironmentVariableReporter in FirstWorkingReporter

Later

• [x] Review tests in tests/DocTest_Tests/reporters/ReporterFactoryTests.cpp - can we remove repetition
• [x] Make it case insensitive - later

[claremacrae]

This code is now on master, and about to be released - thanks @alepez !