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

Fix for issue #1065 NUnit to destroy ActorSystem after each test run inline with XUnit #1092

Conversation

garethbudden
Copy link
Contributor

I've made changes as outlined in #1065 to allow NUnit's TestKit to work in the same way as xunit.

Problems

Providing an ActorSystem in the constructor will fail

Unlike xunit (and I think mstest), NUnit does not create a new instance of the test class for each test run. If the user provides an ActorSystem in the constructor, all tests will attempt to use the same system, however after each test the system is destroyed. One possible fix would be to take a Func<ActorSystem> instead, however that would introduce a breaking change to the API for all TestKit implementations and I wasn't sure how you'd feel about that. Another option is simply to throw a NotSupportedException in the NUnit TestKit if the user provides an instance of ActorSystem.

Redundant methods

The NUnit TestKit has AfterAll(), Dispose(), and Dispose(bool disposing) methods, however these are redundant as the class now uses a TearDown attribute to clean up. Removing these methods makes sense but again introduces a breaking change. Would this be a problem?

@Aaronontheweb
Copy link
Member

Another option is simply to throw a NotSupportedException in the NUnit TestKit if the user provides an instance of ActorSystem.

Code-bombing people is a much better alternative than introducing breaking changes to the API, so I approve :)

@Aaronontheweb
Copy link
Member

The NUnit TestKit has AfterAll(), Dispose(), and Dispose(bool disposing) methods, however these are redundant as the class now uses a TearDown attribute to clean up. Removing these methods makes sense but again introduces a breaking change. Would this be a problem?

The TestKit API is stable as of 1.0. There's no possibility of changing it. NUnit must work with it.

@Aaronontheweb
Copy link
Member

@garethbudden this looks great - just finished my review.

Could you have AfterAll / Dispose etc called inside the testfixture's TearDown-decorated methods? That should work.

@garethbudden garethbudden force-pushed the 1065-destroy-actorsystem-after-each-test-run branch from fc4e5a7 to d7b1c8f Compare June 25, 2015 09:46
@garethbudden
Copy link
Contributor Author

I've added the code 💣, and made the TearDown method call AfterAll().

I haven't made the TearDown method call Dispose() however, because I realised this would mean that the test fixture instance would be Disposed multiple times, one for each test! Which felt wrong, and may cause adverse affects on people who rely on dispose being called once when the Fixture finishes.

@Aaronontheweb
Copy link
Member

Ty sir!

Aaronontheweb added a commit that referenced this pull request Jun 26, 2015
…fter-each-test-run

Fix for issue #1065 NUnit to destroy ActorSystem after each test run inline with XUnit
@Aaronontheweb Aaronontheweb merged commit df7e34c into akkadotnet:dev Jun 26, 2015
@mchandschuh
Copy link
Contributor

@Aaronontheweb -- nunit 3.13 adds [FixtureLifeCycle(LifeCycle.InstancePerTestCase)] which creates a new test class instance per test, but the not supported exception prevents it's use. What needs to be updated to allow this to work?

https://docs.nunit.org/articles/nunit/writing-tests/attributes/fixturelifecycle.html

@Aaronontheweb
Copy link
Member

@mchandschuh we might need to patch Akka.NET first in order to fix this.

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.

3 participants