Adds a dotnet test --logger to invoke Formatter

[clrudolphi]

🤔 What's changed?

This PR adds a custom vstest infrastructure logger that can be used via the command line to configure Formatters.
It will pick up and use the TestResults folder if one is not explicitly specified.

ex:

dotnet test --logger:"formatter-message;outputFilePath=foo.ndjson"

⚡️ What's your motivation?

This does get us access to the TestResults path which otherwise isn't easily available.

🏷️ What kind of change is this?

• ⚡ New feature (non-breaking change which adds new behaviour)

♻️ Anything particular you want feedback on?

Anything.

📋 Checklist:

• I've changed the behaviour of the code
  • I have added/updated tests to cover my changes.
• My change requires a change to the documentation.
  • I have updated the documentation accordingly.
• Users should know about my change
  • I have added an entry to the "[vNext]" section of the CHANGELOG, linking to this pull request & included my GitHub handle to the release contributors list.

---

This text was originally taken from the template of the Cucumber project, then edited by hand. You can modify the template here.

[clrudolphi]

This might be viewed as a bit of a hack, as it takes the configuration that the dotnet test command line provides and formats that into an Environment Variable value that then gets passed to Reqnroll and which controls the generation of Messages. It temporarily overrides the existing REQNROLL_FORMATTERS environment variable; and returns its value after test execution.

[gasparnagy]

Did a research: also could not find a generic way to access the output folder (shame...).

So I was thinking about this formatter implementation. It could be a pretty good convenience feature. Not so match because of the output folder, but because of convenience on the CI servers, so that you don't need to set environment variables or alter the json config to get a simple html report. But for that, specifying a JSON in the command line is cumbersome. I had the following ideas for the configuration via logger:

1. Allow to specify only one formatter: --logger "formatter;name=html;outputFilePath=foo.html"
2. Make this one specific to the built-in formatters fore even more convenience: --logger "html-formatter;outputFilePath=foo.html" --logger "message-formatter;outputFilePath=foo.html"

I don't think that it is a big problem if you cannot configure multiple formatters via the logger infra, but anyhow we could keep the JSON setting as a backup: --logger "formatters;config={...full JSON config here}"

Also we could consider also allowing the LogFileName setting as an alias for outputFilePath. The LogFileName is pretty standard for all loggers.

(The technical implementation with the env var is not necessarily a hack, but probably I would set an internal env var (REQNROLL_FORMATTERS_LOGGER) so that it does not interfere with the one set by the user.)

What do you think?

[clrudolphi]

Agree on all your points. I like the first option.

As an aside, another use for a custom logger would be for us internally. We often struggle to log the internal operation of Reqnroll and to keep such log content separate from the content of the test execution. If we generalized the concept of the IFormatterLog and sent that content only to a custom --Logger, that would provide us a convenient way to log internal execution state.

[clrudolphi]

Another thought- I agree that the logger should set a distinct environment variable.
Should that value overwrite the configuration for the respective formatter OR be in addition to it?

In other words we might have one message sink configured via json file and another configured via command line? I could see a configuration provider that watches for the special environment variable and then adds a new instance of the formatter to the global container. As long as this happens early enough in the startup sequence it would work.

[gasparnagy]

Another thought- I agree that the logger should set a distinct environment variable. Should that value overwrite the configuration for the respective formatter OR be in addition to it?

In other words we might have one message sink configured via json file and another configured via command line? I could see a configuration provider that watches for the special environment variable and then adds a new instance of the formatter to the global container. As long as this happens early enough in the startup sequence it would work.

I think this should be additive/overwrite:

1. If I configured html from JSON, but use --logger with message: it should enable both
2. If I configured html from JSON to save file to x.html, but use --logger with file y.html: it should keep html enabled, but overwrite the output to y.html.

The interesting situation is, what should be the result, if I have x.html configured in JSON (that means bin/Debug/net9.0/x.html), but would "enable" is also from the logger, but without an output file, then it should be

1. Keep it bin/Debug/net9.0/x.html
2. Use the output folder from logger: TestResults/x.html
3. Use the default output file from logger: TestResults/reqnroll_report.html

I think we should go for 1). That is the easiest anyway, and the use-case is pretty much artificial.

[clrudolphi]

I think this should be additive/overwrite:

Let's clarify our rules and order of precedence.

1. We treat the configuration of each formatter separately.
2. As we configure a formatter, a configuration provider with higher precedence may (upsert behavior):
   • override an existing configuration for the given formatter or
   • add a new configuration for a formatter that was not previously configured
3. Configuration providers that don't explicitly provide a directory path do not override the directory that had been previously configured.
4. Precedence order: json < EnvironmentVariable < logger < DisableSwitchEnvironmentVariable

One way to implement the above is to treat the path and filename as independent settings that have recognized default values and those values may be overridden. {Path default: '.', Filename default: 'reqnroll_report.ext'}

Questions:
A) The current implementation uses a single Environment Variable (REQNROLL_FORMATTERS) to configure settings. Should this be changed to one variable per formatter (with a naming convention for the Environment Variable name such as REQNROLL_FORMATTER_HTML?
B) The current Environment Variable format uses json. Should this be changed to key-value pairs? name=html;outputFilePath=reqnroll_report.html
C) I have doubts about 3) - one of the advantages of adding --logger support is the access it gives us to the path to the TestResults directory (without the user needing to explicitly configure it as such). If the user adds a --logger with an outputFilePath that contains just a filename, is it not implied that they want to use the preconfigured TestResults output folder?
D) What is the desired behavior when two or more configuration providers explicitly specify non-default values for the path and/or filename?

[gasparnagy]

Let's clarify our rules and order of precedence.

1. We treat the configuration of each formatter separately.

2. As we configure a formatter, a configuration provider with higher precedence may (upsert behavior):

   • override an existing configuration for the given formatter or
   • add a new configuration for a formatter that was not previously configured

3. Configuration providers that don't explicitly provide a directory path do not override the directory that had been previously configured.

4. Precedence order: json < EnvironmentVariable < logger < DisableSwitchEnvironmentVariable

One way to implement the above is to treat the path and filename as independent settings that have recognized default values and those values may be overridden. {Path default: '.', Filename default: 'reqnroll_report.ext'}

Let's separate this two questions (individually configurable formatters; separation of output folder and output file) and let's concentrate on the "individually configurable formatters" in this PR. So for now let's keep the single outputFilePath setting and not make the output folder separately settable.

So my revised list:

1. We treat the configuration of each formatter separately.
2. As we configure a formatter, a configuration provider with higher precedence may (upsert behavior):
   • override an existing configuration for the given formatter or
   • add a new configuration for a formatter that was not previously configured
3. Configuration providers that don't explicitly provide full path as outputFilePath, should be interpreted from our default output folder bin/Debug/net8.0.

• When --logger is used with an output file setting, it should specify a full path by combining the output folder of the logger infra with the file path specified by the user (e.g. if user specified Foo\Bar.html, the logger provider sets the outputFilePath to C:\...\MyProject\TestResults\Foo\Bar.html)

4. Precedence order: json < EnvironmentVariable < logger < DisableSwitchEnvironmentVariable

Questions: A) The current implementation uses a single Environment Variable (REQNROLL_FORMATTERS) to configure settings. Should this be changed to one variable per formatter (with a naming convention for the Environment Variable name such as REQNROLL_FORMATTER_HTML?

This would be nice IMHO if not much work.

B) The current Environment Variable format uses json. Should this be changed to key-value pairs? name=html;outputFilePath=reqnroll_report.html

If we could do A) then this one would be also nice.

C) I have doubts about 3) - one of the advantages of adding --logger support is the access it gives us to the path to the TestResults directory (without the user needing to explicitly configure it as such). If the user adds a --logger with an outputFilePath that contains just a filename, is it not implied that they want to use the preconfigured TestResults output folder?

Let's not deal with this now, see above.

D) What is the desired behavior when two or more configuration providers explicitly specify non-default values for the path and/or filename?

I don't understand this question.

[clrudolphi]

I've adopted your comments and have it almost working correctly.
The limiting factor now is that --logger does not support being listed multiple times with the same logger name. That is:
--logger "formatter;name=message;outputFilePath=foo.ndjson" --logger "formatter;name=html;outputFilePath=foo.html"
will result only in the second logger taking effect.
To my understanding, there is no way to register a logger using a dynamic name (such as --logger "formatter-html"). We could create logger classes for the two built-in loggers, but this mechanism would not work with any user-supplied formatters.

How would you like to proceed? Options:

• fall back to using string encoded json instead of kvp;
• support only a single logger via the command line
• create built-in loggers for our two built-in formatters
• both 1 and 3 (built-in loggers and support for json)

[clrudolphi]

As the first portion of refactoring this, commit f99c7d3 creates two named loggers (one for messages and one for html).
Usage:
--logger "formatter-html;outputFilePath=foo.html" --logger "formatter-message;outputFilePath=foo.html"

If you prefer the names reversed (such as html-formatter) let me know.
These support configuring the file path in KVP form.

I'll next add another more generic formatter logger (will call it formatters {plural}) and require the configuration string to be encoded json. This form will be able to support any combination of built-in and user-plug-in formatters.

[clrudolphi]

@gasparnagy I've tried to create a --logger that would accept a general json config element and I'm failing. I can't get the quote escaping correct. Powershell and/or the dotnet test infrastructure seem to strip out all quotes from the command line parameter and I'm left with a string that I can't pass to the Formatter configuration subsystem. I don't have confidence that I could parse this string and reconstruct a proper json representation of it (as its supposed to be general, we can't predict what other plugin formatters may want as configuration input).

So far, I've tried:

• surrounding the string with single-quotes
• escaping quotes with back-ticks:

--logger 'formatters;config={`"formatters`":{`"message`":{`"outputFilePath`":`"foo.ndjson`"}}}'

• doubling up the double-quotes (as in, --logger "formatters;config={""formatters"":{""message"":{""outputFilePath"":""foo.ndjson""}}}"
• using powershell variables

 $config = '{"formatters":{"message":{"outputFilePath":"foo.ndjson"}}}'
PS C:\Users\clrud\source\repos\scratch\vs95\vs95> $config
{"formatters":{"message":{"outputFilePath":"foo.ndjson"}}}
PS C:\Users\clrud\source\repos\scratch\vs95\vs95> dotnet test -c Debug --logger "formatters;config=$config" 

I'm open to suggestions of things to try. Any ideas?

[gasparnagy]

@clrudolphi sorry for not responding earlier.

As the first portion of refactoring this, commit f99c7d3 creates two named loggers (one for messages and one for html). Usage: --logger "formatter-html;outputFilePath=foo.html" --logger "formatter-message;outputFilePath=foo.html"

This is great! I like it.

If you prefer the names reversed (such as html-formatter) let me know. These support configuring the file path in KVP form.

I see the problem. Not easy. Probable the reverse name would be better though.

I've tried to create a --logger that would accept a general json config element and I'm failing.

I have also tried that once (other project), but running into the same issues. I think this is not feasible, because others will suffer from the same problems.

My suggestion is the following:

The generic one should be called "formatter" (singular) and should only allow configuring a single formatter, in a form --logger "formatter;name=message;outputFilePath=foo.ndjson". As we have specific one for html and message, it is unlikely that someone want to configure two custom formatters from the command line, but if so, they can still do it via environment variables.

[clrudolphi]

If you prefer the names reversed (such as html-formatter) let me know. These support configuring the file path in KVP form.

I see the problem. Not easy. Probable the reverse name would be better though.

No problem at all. I'll make that change.

I've tried to create a --logger that would accept a general json config element and I'm failing.

I have also tried that once (other project), but running into the same issues. I think this is not feasible, because others will suffer from the same problems.

My suggestion is the following:

The generic one should be called "formatter" (singular) and should only allow configuring a single formatter, in a form --logger "formatter;name=message;outputFilePath=foo.ndjson". As we have specific one for html and message, it is unlikely that someone want to configure two custom formatters from the command line, but if so, they can still do it via environment variables.

The whole point, really, of the generic logger was to support plugin formatters. IMO, we have sufficient mechanisms for the built-in ones.
The problem (?) with using only kvp is that it might not allow for adequate expressiveness for future plugins. And if the plugin formatter were to require a string that requires some kind of character escaping, then I question whether it is feasible to configure via the --logger command line at all (given the restrictions and complications that we can't overcome) {{eg, what if a future plug-in formatter wanted to push the results to a web address; how feasible would it be to escape the URL within the --logger string?}}

Therefore, IMO, we drop the idea for now. I'll make the adjustment to the named formatter loggers and then we're done.

[clrudolphi]

Before we merge this, I'll edit the docs to include the logger configuration of the formatters.
@gasparnagy And before that, would you mind reviewing the PR #734 with documentation edits that I made earlier today? Those should probably be merged before I edit the doc again.

Edit: I put the revised docs into the page that describes other loggers. There is no merge conflict with the Formatters documentation page.

[obligaron]

Just a question:
Does this also work with the new Microsoft.Testing.Platform?
Do we need a test for both (vstest and Microsoft.Testing.Platform)?

For reference see discussions in Tyrrrz/GitHubActionsTestLogger#41.

[clrudolphi]

Ugh. No good deed goes unpunished.

[gasparnagy]

Hm. I have checked the linked article, and based on that we should strongly consider if we want to have this logger support included. It is not essential for us and it is declared to be slowly deprecated.

Later we can think about having an MTP reporter.