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 361 #39

Merged
merged 2 commits into from
Mar 22, 2013
Merged

Fix for issue 361 #39

merged 2 commits into from
Mar 22, 2013

Conversation

bittailor
Copy link
Contributor

Issue 361: NullReferenceException when subscribing to an event

Please see https://code.google.com/p/moq/issues/detail?id=361 for details.

Issue 361: NullReferenceException when subscribing to an event

See https://code.google.com/p/moq/issues/detail?id=361 for details
}

[Fact]
public void ReproduceBug()
Copy link
Member

Choose a reason for hiding this comment

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

Once the bug is fixed, what is the assertion to make here? Is it that it doesn't throw? Or some actual expected behavior?

@bittailor
Copy link
Contributor Author

It should just not throw an exception.

@kzu
Copy link
Member

kzu commented Mar 21, 2013

When the bug is fixed, the fixture and its fact methods will not tell us anything (from their name) about what was the expected behavior.
I'd suggest renaming the method to something like "ShouldSuccessfullyAttachToProtectedEvent" or the like.
And move the tests to the MockedEventsFixture.cs flie which is the one that contains events tests.

Thanks!

@bittailor
Copy link
Contributor Author

Fact renamed and moved to MockedEventsFixture.cs.
Also added a Assert.DoesNotThrow so the test has an explicit assertion.

kzu added a commit that referenced this pull request Mar 22, 2013
Fix for issue 361

Looks great :)

Thanks!
@kzu kzu merged commit 6021e28 into devlooped:dev Mar 22, 2013
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

2 participants