Add diagnostic message when JSON file is malformed

[bradwilson]

Related: #1654

[dnikolovv]

I can take that one, but how do you suggest for it to be implemented.

An analyzer seems to suit best, but you could also throw an exception during ConfigReader.Load.

[mehdikhaleghian]

I was looking into this as well? how do you suggest it to be implemented?

[skimmedsquare]

Seeing as the last two comments offering to pick this work up from 2019, I started working on a PR for this, should have it posted sometime tomorrow.

[ghost]

@bradwilson Hey, I took a look at this issue as I was thinking of taking it and the (now stalled) pr you commented on and while I think there are ways to handle this, I think it comes down to have the runner receive an exception it handles on ConfigReader.Load.

Where I am unsure is it looks to me as if this method was designed to have a distinction between failing to load, but without needing to report it and failing to load due to a problem that does need the runner's attention (such as is the case with this issue). The problem is checking usage, no one seems to care about the actual bool return except the utility runner's tests. This makes the intention ambiguous: do we care about whether or not it succeeded or we care about why it failed?

I do agree with the pr's author that returning a null string seems odd: it doesn't sound obvious to me what such a string would represent. How about changing the method to TryLoad instead? It would return a bool, but take in a ref string? that tells you the error or null if no errors. This would cover all three cases (success, failure without error or failure with errors), the runners would then need to handle it on their own.

Alternatively, if any failure is considered an error, then I think the better approach would be to throw. In such case, who is responsible for catching? The runner? Incidentally, this approach has the advantage that little signature changes are needed because the exceptions can be bubble up to the runner.

Once I have a clearer ideas on these, I could move forward and submit a pr superseding the previous one.

[bradwilson]

I haven't really thought much about this since the previous PR, so I went back and reviewed it.

I think the best thing would be to just add something like out List<string> warnings to give the caller a list of messages to show to the user. Those could get reported as warnings to the user. A malformed JSON file would definitely fall into this category.

Later if users desire, we could add a switch to runners to say "treat config warnings as errors" so that it would fail the run rather than just ignore the problems.

The complexity comes in because of the existing ConfigReader API and optional parameters.

v2

The two APIs today are:

TestAssemblyConfiguration Load(string assemblyFileName, string configFileName = null)
TestAssemblyConfiguration Load(Stream configStream)

I think we'd want to obsolete the old overloads, implemented as calls through to the new APIs, and just throw away the warnings. The new APIs would be:

TestAssemblyConfiguration Load(string assemblyFileName, string configFileName, out List<string> warnings)
TestAssemblyConfiguration Load(Stream configStream, out List<string> warnings)

v3

In v3, we've already consolidated down to a single API:

bool Load(TestAssemblyConfiguration configuration, string? assemblyFileName, string? configFileName = null)

Since v3 hasn't shipped yet, so no need to do obsoletes, just convert it to this:

bool Load(TestAssemblyConfiguration configuration, string? assemblyFileName, string? configFileName, out List<string> warnings)

(I'm not 100% sure why I don't have a Stream overload here, but we can ignore that for now.)

Thoughts?

[ghost]

I haven't really thought much about this since the previous PR, so I went back and reviewed it.

I think the best thing would be to just add something like out List<string> warnings to give the caller a list of messages to show to the user. Those could get reported as warnings to the user. A malformed JSON file would definitely fall into this category.

Later if users desire, we could add a switch to runners to say "treat config warnings as errors" so that it would fail the run rather than just ignore the problems.

The complexity comes in because of the existing ConfigReader API and optional parameters.

v2

The two APIs today are:

TestAssemblyConfiguration Load(string assemblyFileName, string configFileName = null)
TestAssemblyConfiguration Load(Stream configStream)

I think we'd want to obsolete the old overloads, implemented as calls through to the new APIs, and just throw away the warnings. The new APIs would be:

TestAssemblyConfiguration Load(string assemblyFileName, string configFileName, out List<string> warnings)
TestAssemblyConfiguration Load(Stream configStream, out List<string> warnings)

v3

In v3, we've already consolidated down to a single API:

bool Load(TestAssemblyConfiguration configuration, string? assemblyFileName, string? configFileName = null)

Since v3 hasn't shipped yet, so no need to do obsoletes, just convert it to this:

bool Load(TestAssemblyConfiguration configuration, string? assemblyFileName, string? configFileName, out List<string> warnings)

(I'm not 100% sure why I don't have a Stream overload here, but we can ignore that for now.)

Thoughts?

This seems fine by me.

One more thing for me to at least do the v3 part: how do each runners emit warnings? Is there a standard shared way like an interface between hem to do this?

[bradwilson]

how do each runners emit warnings? Is there a standard shared way like an interface between them to do this?

There is one, IRunnerLogger (v2, v3), but it's intended as an interface between runners and reporters, and the runner provides the implementation of it so the reporter can send messages. The runners will use the logger to log messages as well, because it often centralizes things like locking/synchronization when needed. Today, it's only a requirement that runners implement this if they want to support reporters.

• The console runners share ConsoleRunnerLogger
• The MSBuild runner uses MSBuildLogger
• The TestDriven.NET runner does not have one, because it doesn't support reporters
• The VSTest adapter (which is still using v2 code) uses VisualStudioRunnerLogger

[bradwilson]

Available in v2 runners: 2.6.7-pre.7
Available in v3 runners: 0.1.1-pre.353
Available in xunit.runner.visualstudio: 2.5.7-pre.10

https://xunit.net/docs/using-ci-builds