nunit / docs

Documentation for all active NUnit projects
https://docs.nunit.org
MIT License
602 stars 153 forks source link

ITestEventListener will not be recognized if you follow the documentation #111

Closed olsch closed 8 years ago

olsch commented 8 years ago

Don't use [TypeExtensionPoint(Description ="...")] public class Class1 : ITestEventListener { ... }

Instead do this [Extension(Description ="...")] public class Class1 : ITestEventListener { ... }

CharliePoole commented 8 years ago

You have not indicated where you found the documentation that you believe is incorrect. I can't find it. :-)

FYI TypeExtensionPoint is used to define ExtensionPoints while Extension is used to define Extensions. ExtensionPoints are defined within NUnit. They are the things you can extend by defining an Extension.

Based on your usage, I think you are trying to define an extension and so should use ExtensionAttribute but if there is anything in the docs that misled you I'd like to fix it.

olsch commented 8 years ago

https://github.com/nunit/docs/wiki/Event-Listeners

CharliePoole commented 8 years ago

That's where I had looked, but I didn't find any problem. It gives this example of an extension...

[Extension]
public class MyEventListener : ITestEventListener
{
    ...
}

After that it shows the definition of the ITestEventListener interface. I think we have to show that since you are required to implement it. That interface naturally has a TypeExtensionPointAttribute on it. I'm guessing this is what confused you.

Can you suggest what we might do to avoid confusion by others? The only thing that comes to my mind is to add a // Your code here comment to the example code.

olsch commented 8 years ago

Certainly, maybe my misunderstanding is because of this post http://stackoverflow.com/questions/38749270/how-to-use-itesteventlistener-in-nunit-3 but I failed to make NUnit.Engine discover my extension until I explicitly declared [Extension(Description ="...")]. I think you need do make that clear in the documentation.

olsch commented 8 years ago

This works:

using System.IO; using NUnit.Engine; using NUnit.Engine.Extensibility;

namespace ClassLibrary1 { [Extension()] public class Class1 : ITestEventListener { public void OnTestEvent(string report) { //Do something with "report" } } }

Everything else doesn't

CharliePoole commented 8 years ago

That SO answer is my bad. I never noticed that the poster used TypeExtensionPoint rather than Extension! I'll add a comment to it.

Are you saying that the EngineVersion property is being required? I'll have to go back to the code, but I'd say it should be optional. Same thing with Description.

EngineVersion is new and isn't even in the docs, so I'll have to fix that. It only says "don't try to load this for lower versions". What engine version are you using?

olsch commented 8 years ago

I'm using 3.4 and I'm pretty sure EngineVersion is optional. However, I think it's necessary to correct the documentation and make clear that ExtensionAttribute must be used.

CharliePoole commented 8 years ago

Right. But I'm asking for a suggestion as to how to correct it. It may be because I wrote it, but it seems to me that it already says you should use ExtensionAttribute.

olsch commented 8 years ago

Maybe you could clarify that implementations of ITestEventListener will be discovered only if they have the attirbute "ExtensionAttribute". That clarification should perhaps be in the section "Locating Addins" on https://github.com/nunit/docs/wiki/Engine-Extensibility which should also be referred to on https://github.com/nunit/docs/wiki/Event-Listeners . It would have helped me :-)

CharliePoole commented 8 years ago

I see what you mean. I think the real problem is that we don't have a user doc about how to write an engine extension. The page at https://github.com/nunit/docs/wiki/Engine-Extensibility is really documenting the internal architecture of NUnit.

As a first change, I updated that page to say - I hope clearly - that it isn't user documentation and I moved it to the internal architecture part of the menu. Of course, now there isn't anything obvious for the user, so that's the next step.

CharliePoole commented 8 years ago

The new Writing Extension Points page will have user-oriented information without all the extra detail about NUnit's internal architecture. We knew we needed this but I just had not yet gotten around to writing it. Currently, the page just has a small part of what's was in the Engine Extensibility page, but I'll get it updated later tonght or tomorrow.

olsch commented 8 years ago

Great, thanks!

CharliePoole commented 8 years ago

@olsch @nunit/core-team Please review and comment on the new paage Writing Engine Extensions.

https://github.com/nunit/docs/wiki/Writing-Engine-Extensions

olsch commented 8 years ago

The new page is much better. However, I think the documentation could be more simple and clear. There is no need for us who want to write an extension to understand the concept of Extension Points. The only thing we need to know is that there are four interfaces available if we need to make an extension. Perhaps documentation could be divided into three distinct parts: One for test writers, one for extension writers, and the third for those who need to understand the internals of NUnit.

CharliePoole commented 8 years ago

Well, there are already three distinct parts... Most of the documentation is for users. The section titled "Extending NUnit" is for extension writers and the section "Internal Architecture" is about the internals of NUnit. The last one may end up getting moved into "Developer Docs."

I'm not sure I can see how you might write an extension without knowing what points are available to be extended, i.e. Extension Points. How do you personally think about this? That is, if I ask "What are you extending in NUnit?" what would be your answer?

One problem might be that I'm thinking of all possible present and future extension points. NUnit will definitely add more extension points and users will be able to define extension points in their extensions - allow the extensions to be extended themselves. OTOH, the current state of extensibility is that we have only four extension points and they happen to map one-to-one to specific interfaces. That, however is an accident of our early stage implementation.

It's also likely that a Test Event Listener is the easiest of the four to understand without reference to extension points because it's such a general interface. But because of that it's also the most likely of the four interfaces to be reused by additional extension points. In fact, that already happened once, with the TeamCity extension. However, we revised it to use the same extension point before publication and so there is so far no ambiguity.

It could be that I'm presenting things on the page in the wrong order. Since many extensions can be written knowing only the interface, maybe Extensions should be described before Extension Points. I'll give that a try.

olsch commented 8 years ago

I fully understand that Extensions Points offer much more than what is currently in NUnit. But I am pretty sure that the majority of Extensions writers want to to know what they can do right now. That's why I think you should separately describe the four interfaces currently available and, for those interested, provide a link to a more in-depth explanation of Extension Points. Personally I would have been happy with something like this: "If you need to listen to specific events during the running of a test you need to impement ITestEventListener. The class implementing ITestEventListener must also be marked with the Extension attribute, so that NUnit Engine can discover your extension." I know that all this already is in the documentation, but I think it can be clarified.

CharliePoole commented 8 years ago

I'll give that some serious thought.

In a book (which I have thought about) I would write a chapter about event listener extensions. I might not write about any others, since it's extremely likely that most of the users who write extensions (a small number anyway) will write event listeners. Very few people will turn around and decide to extend NUnit so it runs, for example, xUnit tests.

OTOH, this is reference documentation, so I do feel obligated to have everything in it somewhere, although not necessarily in the same page.

olsch commented 8 years ago

This is where as much details as possible should be documented of course. Hopefully I have helped a little how to structure a small part of the documentation. I am looking forward to reading your book.

CharliePoole commented 8 years ago

You've helped quite a lot, thank you.

There is now a final update of the page. I decided to remove all details of extension points, although the term continues to be used, and I'm relying on the separate pages for each type of extension to give further info rather than repeating it on the main page.

I think it's quite a lot better now.

olsch commented 8 years ago

Thank you, and I agree, it's much better now :-)

CharliePoole commented 8 years ago

@olsch I'm closing this issue. FYI, I just posted an article on my blog about writing a different type of extension: http://nunit.net/blogs/?p=74

olsch commented 8 years ago

Thanks Charlie, for great feedback on the issue and for your blog post!

:-)

//Olov

2016-09-23 0:05 GMT+02:00 CharliePoole notifications@github.com:

@olsch https://github.com/olsch I'm closing this issue. FYI, I just posted an article on my blog about writing a different type of extension: http://nunit.net/blogs/?p=74

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nunit/docs/issues/111#issuecomment-249042040, or mute the thread https://github.com/notifications/unsubscribe-auth/AIoimpYassaITSAPoJLtgRLGdr8C7z0Lks5qsvvGgaJpZM4J0FGw .

mraphael2101 commented 3 years ago

This works:

using System.IO; using NUnit.Engine; using NUnit.Engine.Extensibility;

namespace ClassLibrary1 { [Extension()] public class Class1 : ITestEventListener { public void OnTestEvent(string report) { //Do something with "report" } } }

Everything else doesn't

Works for me. Thanks.