microsoft / testfx

MSTest framework and adapter
MIT License
697 stars 250 forks source link

Add stdout/stderr properties to TestNode #3225

Closed drognanar closed 3 weeks ago

drognanar commented 1 month ago

Add stdout/stderr properties for the TestNode to the spec.

MarcoRossignoli commented 1 month ago

I'm not sure how this is possible for an adapter without:

I was wondering if make more sense have something more high level and less "low level" like notes-info/notes-error/notes-warning (naming TBD)

cc: @Evangelink

drognanar commented 1 month ago

I'd be fine renaming it to output.info and output.error accordingly, to reflect closer that it's the output of that test, rather than notes.

If more levels are needed, or if the test should have a message stream we could consider that too. For instance:

{
  "output": [
    { "level": "info", "message": "Message 1" },
    { "level": "error", "message": "Error 1" },
    { "level": "info", "message": "Info and errors can interleave this way, rather than have to be two separate streams" },
    { "level": "error", "message": "Clients may still combine the outputs, but some clients might do a better job at interleaving those." },
  ]
}

As for how this is generated it should be up to the framework to decide. If it only wants to support an internal ITestOutput abstraction, it should be allowed to do so. If it wants to support Console.Out/Error redirection then it is fine. The protocol is simply stating that if these properties are set they should be handled by the clients and shown in the results UI.

Evangelink commented 1 month ago

I think the point of @MarcoRossignoli is more to question having some "output" related to a "test node". You are taking a specific use-case and making it a component of the protocol that is supposed to represent a functional abstraction. I think it's better to think that a node can have messages attached (whether this is an output or something else is an implementation detail).

drognanar commented 1 month ago

I think the point of @MarcoRossignoli is more to question having some "output" related to a "test node".

If we rename output to messages or logs does it make the meaning of the property abstract enough? messages wouldn't be linked to stdout/stderr, and more generally would encompass any loggable messages, allowing adapters to log additional diagnostic messages too. On the other hand, the property name notes feels too disconnected from logging concepts.

@evangelink do you have a preference of which of the two styles would be better. Whether { "messages.info": ..., "messages.warning": ..., "messages.error": ... } or { "messages": [ListOfMessages] }

Evangelink commented 1 month ago

allowing adapters to log additional diagnostic messages too

Is this problematic? You have a point that we do need different "levels" and that maybe logging is indeed more generic.

drognanar commented 1 month ago

After an offline discussion, I updated the spec to be based on the client/log rpc instead and support log message association to the specific runId/nodeUid couple. This should allow batching of logged messages, support more logging levels than just output/error and allow to send output updates during a test run without the need to resent the entire TestNode all the time.