Include type in start-suite/start-test report elements

[chrisalbrecht]

It would really help me if start-suite/start-test reports emitted by the TestProgressReporter included the TestType.

This seems to do the trick, but I'm wondering if I'm missing some reason that TestType isn't already part of ITest?

[CharliePoole]

There's no issue for this change, so we will have to treat the PR as if it were the issue and decide if we want to implement the suggested feature.

At first glance, this seems very odd. The intent of the start-test and start-suite events is purely to indicate the identity of the test/suite which is starting. This is available for use, for example, in a GUI that needs to display the name or possibly color the node representing any test currently in execution. It's not intended to give other info about the test because any program (like a GUI) that might need that information, already has it available because it has loaded a complete description of all the tests.

Clearly, you have some use case for the test type in mind. Can you please explain it?

[chrisalbrecht]

Sure. We've created a build telemetry framework to help us collect near real-time metrics on all of the steps/events that occur in our CI builds.

To collect details on our test runs we've created an extension that implements ITestEventListener and relays the events to an SQS queue for further processing.

We'd like to be able to do some filtering based on the type of the test event, but this property is currently missing from the start events.

Hope this helps. Let me know if you'd like me to create an issue to reference in this PR.

[rprouse]

We don't need an issue for this, we can discuss it here and treat it as if it was an issue. The change is simple enough, although it doesn't have any unit tests.

My main concern is that this is a very niche requirement that probably only applies to your CI builds. How would this be useful to the wider NUnit community?

[chrisalbrecht]

While this may indeed be a niche requirement, isn't the purpose of providing extension points to support the unique needs of those in community? I feel that the richer the interfaces are, the greater the possibility for new and innovative uses. I'd like to believe that if I can benefit from this change, its not that unlikely that someone else can as well.

[rprouse]

@chrisalbrecht good point. I am not really against this change, but I also don't like feature creep. This is additive and doesn't break existing consumers so i am okay. @CharliePoole are you satisfied with his use case?

[rprouse]

@CharliePoole can you take another quick look at this PR and see if @chrisalbrecht's comments satisfy your concerns?

[CharliePoole]

I can live with it. 😄

The general idea is that the start-test message should provide identification of what is starting, not info about the test. Explore is available for runners that want to know more, such as guis. To that extent, @chrisalbrecht hasn't provided any info about why his runner cannot use the existing information but requires another field to be added to the start message.

OTOH, it's not out of the question to view the type attribute as part of the identification of a test along with id, name and fullname. After all, if we wanted to be completely purist about it we would only have the id, from which you can look up everything else. :-smile:

I would be pretty adamantly against adding any other characteristics of the test to the start message, however.

[rprouse]

@CharliePoole based on that, we just need a second code review. Can you take a look and merge when you are at a computer?

[rprouse]

Since there is no issue with this, I have added a milestone and labels so it gets tracked with the release.