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

Dump xml #403

Merged
merged 3 commits into from
Oct 15, 2017
Merged

Dump xml #403

merged 3 commits into from
Oct 15, 2017

Conversation

OsirisTerje
Copy link
Member

@OsirisTerje OsirisTerje commented Oct 6, 2017

This PR will add support for generating xml file outputs, aimed at debugging and investigations, but can surely be used otherwise too.
It works now for .net fw, but not currently for .net core, need a touch on the xml text writing, which is what is stopping it there.

It is controlled by two new settings in the runsettings file,
DumpXmlTestDiscovery
DumpXmlTestResults

It will output the xml files into the same folder as the assembly under test, into a subfolder named "Dump", and with a filename equal to the assembly name, but with the .dump extension
Further, the discovery file is prefixed with "D_" and the execution file with "E_"

The execution file will contain anything found during start of the run, and also anything received through the run events.

@CharliePoole
Copy link
Member

@OsirisTerje I don't understand why we want to write code to do this, rather than just telling the engine to do it. Also, you are not following the convention that output goes to the designated WorkDirectory. Since my comment is pretty general, I have not yet looked at the actual code.

@OsirisTerje
Copy link
Member Author

OsirisTerje commented Oct 6, 2017

@CharliePoole Simply because I was not aware that the engine could be told, and I have been doing debugging to get at these data lately. I have asked about these data many times before, and am a little put off if this facility already exist in the engine. It should then have been exposed in the adapter.
This is information that would have been very much more helpful earlier.
OTOH: The code to do this is trivial.

Further, I would like to see how the data comes out, before setting particular directories. Workdirectory is fine, this is PR so things can be changed. Again, the workdirectory is something that is being set in the code, I didn't want to figure that out, but is trivial to change.

Note also that the connected issue #323, which is not yet concluded - since it also covers .net core - has been open for a very long time, with no such information provided.

@OsirisTerje OsirisTerje requested a review from rprouse October 6, 2017 23:17
@OsirisTerje
Copy link
Member Author

OsirisTerje commented Oct 6, 2017

@CharliePoole Incidentally , this implementation will also fix #381 . If there is anything in the engine that does this too, please enlighten me.

@CharliePoole
Copy link
Member

@OsirisTerje The engine API is pretty well documented. WRT the services it provides see https://github.com/nunit/docs/wiki/Test-Engine-API#engine-services The table links to the IResultService interface in the source, which allows you to get an IResultWriter for a particular format. In this case, you want the nunit3 format.

This ability was not originally exposed in the adapter because it was thought that the audience for the adapter didn't want NUnit XML results, but would rely on VS to provide output result files. That was the opinion of the VS team with which I originally worked and I didn't have any info to the contrary. Subsequently, we got requests for this as a feature and created issues. The primary issue is #215, which nobody has stepped forward to work on.

I don't really understand your being put off by the fact that the engine has this feature. It isn't a secret, obviously, has been discussed in the relevant issues for a long time and is documented. I assumed you knew about it.

WRT issue #323, my understanding is that it is purely related to the .NET Core runner. I could be wrong there, however, since I'm not familiar with the code.

WRT issue #381, I think that's a different matter because it addresses putting out incremental results as the test progresses. However, if incremental results is what you want, then the engine facility is not what you are after.

@OsirisTerje
Copy link
Member Author

OsirisTerje commented Oct 7, 2017

@CharliePoole No, I was not aware of it, and none of the issues, afaic see, mention this.
But great that it is there.
Now - how do I then format the output ?
How does it handle subfolders?
I want the same exact output format as the adapter sees, including what is coming through the test events.
If I get you right now, because this is not stated in the documentation, it will do that?

To me it seems to only do the serialization and file writing of the results. Handling of file names is not included, but I assume there is another service I can use for that?

Both solutions will have an assembly granularity today, to fix #381 the file needs to be per test case, something that easily can be added to the DumpXml code. It should be likewise easy to add to the engine too, or simply by calling it per test case.

@CharliePoole
Copy link
Member

Sorry, I'm not understanding some of the questions...

"How do I format the output?" The output is a file containing XML. The usual default format is nunit3, which is the unchanged format produced by the framework. Extensions can produce other formats, but we don't have extensions enabled in the adapter.

"How does it handle subfolders?" I don't understand what you mean. The result writer creates a file at the path you specify. Use of the WorkDirectory is a convention only - the file path can be specified in full on each call.

I looked at the issues and I can see you are right - none of them spell out "This can be done by the engine." I guess it was an assumption because most of the people in the discussion were our own team members.

Regarding #381, we have always taken the position that XML is not a good format for a log because logs need to be produced incrementally and XML needs to be properly terminated. Of course, it's a new regime and you are free to change that position.

Bear in mind that I have no info as to what problems you are trying to solve. The only info is the title "Dump XML" so I made an assumption that you want to produce the same output XML that all our other runners produce. If that's not what you are trying to do, then the engine feature isn't useful for you.

@OsirisTerje
Copy link
Member Author

OsirisTerje commented Oct 7, 2017

As stated in the description for this PR, it is aimed at debugging and testing. To be more clear, this PR covers the part of the traits testing which can be seen as separate, thus splitting it out as you requested on #401.

The point here is that I need the exact xml as it comes out from the engine. I don't care if this is the NUnit3 format or something slightly different or changed. I need it as it comes from the engine to the adapter.
I then use the extracted (dumped) xml to copy/paste it "as is" into the test cases for checking - for now - the traits, refer to the mails and issue questions I have had with you wrt to that. This xml represents a major part of the interface to the engine, and I don't want tests to use the engine, but instead use the interface to the engine, including this xml.

With formatting I mean I want it so that 1) it is readable, thus I want it with line breaks (which it doesn't have from the engine) 2) Easy to copy paste in, which means with single quotes instead of the default double quotes (both are acceptable xml). We then copy this into the class named TraitsTest together with a comment of what C# code that produced this xml response from the adapter. 3) We want both the xml from the discovery and from the execution phase. The engine seems to only give the results. For the execution we want both the extracted from the initialisation and from the events that are being fed as it runs.

I would also like to use the same type of xml data to feed other tests, instead of having to rely on faked binaries. This [faked binaries] is something we have seen being used wrongly where we did not intend them to be used. Mocking goes only so far, we need the result produced too, which this feature can help and speed up delivery of.

Further, it can be used or seen as partly logging, but it is not logging per se. Since it outputs the information as it runs, it has a sort of logging character, and I would like to add some small help information so that it can help to a certain degree there. But, it is not intended to be a logging feature. It is a dump.

Then, if this dump can solve part of the request for xml output for other purposes, I am happy with that. But those requests could also be served by calling the resultservice, thus enforcing a standard format. I think that is a better approach to solve those requests.

Did this make sense?

@CharliePoole
Copy link
Member

It sounds as if you intend this for debugging and testing of the adapter itself and not really for the purpose of debugging and testing of the SUT.

If that's the case, then all my comments were a bit misdirected, since I was treating it as a new user feature. Your reference to various user issues helped me to that conclusion. I don't think you should try to kill two birds with one fix, because user needs are different from ours.

It also wasn't clear that you are talking about the XML fragments sent as the test runs, not the final, complete XML. Those fragments are identical to the final XML but are of course just fragments.

In that case, it's not s facility anyone has needed before, but I can see how it might be useful. My only remaining reservation is that you are adding it to me user interface by using the run settings file. Wouldn't it be better to trigger it in some other way?

@OsirisTerje
Copy link
Member Author

It could perhaps be triggered some other way, not sure what that should be, since the VSTest is calling the adapter, and the only thing we have which we know will be read is the runsettings. And, it is simpler than using the registry, which could be one. Further, we have other debugging information there, like Verbosity and InternalTraceLevel, so it is not quite out of context. It could delegated to a subsection there for debugging info.
Even if this is very specific for adapter development, I would rule out the possibility it could help others - at least in understanding why they get some results they wonder about. We have many cases where we do checking to see if whatever fault there is originates in the adapter or the engine/framework. Checking the xml in this manner might help in these cases too.

@OsirisTerje OsirisTerje merged commit d8943d8 into master Oct 15, 2017
@OsirisTerje OsirisTerje deleted the DumpXml branch September 22, 2021 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants