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

Add stdout/stderr properties to TestNode #3225

Merged
merged 4 commits into from
Aug 5, 2024

Conversation

drognanar
Copy link
Member

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

@MarcoRossignoli
Copy link
Contributor

MarcoRossignoli commented Jul 8, 2024

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

  • provide an abstraction on console that needs to be used by the user (ala ITestOutput...)
  • don't substitute the console out soon as possible(if it's possible) and lock somewhere, with all the problem of appdomain and static substitution of the output

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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.

Make use of client/log to log stdout/stderr messages,
but allow these to specify the runId/nodeUid properties
to associate the logged message to a TestNode in a specific
discovery/run request.
@drognanar
Copy link
Member Author

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.

Evangelink
Evangelink previously approved these changes Jul 11, 2024
Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@MarcoRossignoli @nohwnd @jakubch1 please review too

@MarcoRossignoli MarcoRossignoli merged commit 2c9dad5 into microsoft:main Aug 5, 2024
3 checks passed
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.

None yet

3 participants