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 NullReferenceException in tests #139

Merged
merged 3 commits into from
Aug 3, 2018
Merged

Fix NullReferenceException in tests #139

merged 3 commits into from
Aug 3, 2018

Conversation

martincostello
Copy link
Contributor

If multiple unit tests are running in parallel, it is possible for the Add() operation on the dictionary to throw a NullReferenceException internally and cause a test to fail.

Fix this by using ConcurrentDictionary instead.

Originally part of #131.

If multiple unit tests are running in parallel, it is possible for the Add() operation on the dictionary to throw a NullReferenceException internally and cause a test to fail.
Fix this by using ConcurrentDictionary instead.
@martincostello martincostello mentioned this pull request Jul 31, 2018
2 tasks
@StanleyGoldman
Copy link
Collaborator

How are you able to reproduce this NullReferenceException

I'm using NCrunch's churn mode to try to reproduce this test running ExpressionRewriterTests endlessly.
I can't seem to get the failure.

@martincostello
Copy link
Contributor Author

I just use the Visual Studio Test Explorer.

I right click on the .UnitTest node and click Run Selected Tests.

This is from the Update-Test-Target-Framework branch as I don't have .NET Core 1.x installed on my machine so the tests don't run otherwise (it's what motivated me to make the change in #140).

image

@StanleyGoldman
Copy link
Collaborator

image

Finally reproduced

@StanleyGoldman
Copy link
Collaborator

So a sure fire way for me to reproduce this is to run all three test namespaces once with the Test Explorer.
Then to continually use the "Repeat Last Run" function until i get the failure.

@StanleyGoldman
Copy link
Collaborator

That happened for me on the current master

@martincostello
Copy link
Contributor Author

Must be an implicit dependency between the suites doing stuff with the trees, plus the default xunit level of parallelism is to run test assemblies against each other in parallel.

@StanleyGoldman
Copy link
Collaborator

So now i'm testing this pull request using the same means as above, and I'm able to push it to a different race condition. @martincostello wanna see if you can repro/fix this?

image

@martincostello
Copy link
Contributor Author

Interesting - I didn't get that one. I'll take a look in a bit and see if I can make that case behave too.

Prevent sourceExpression being changed between initialisation and assignment.
@martincostello
Copy link
Contributor Author

@StanleyGoldman Want to pull the latest commit and give that a try?

@StanleyGoldman
Copy link
Collaborator

I'm unable to get it to crash with these changes. Good 💩.

Copy link
Collaborator

@grokys grokys left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @martincostello!

@StanleyGoldman StanleyGoldman merged commit 74c95b9 into octokit:master Aug 3, 2018
@martincostello martincostello deleted the Fix-Test-Concurrency-Issue branch August 3, 2018 14:48
@StanleyGoldman StanleyGoldman added this to the v0.1.1-beta milestone Aug 10, 2018
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