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

Issue 1181 #1268

Merged
merged 2 commits into from
Feb 15, 2016
Merged

Issue 1181 #1268

merged 2 commits into from
Feb 15, 2016

Conversation

ChrisMaddock
Copy link
Member

This catches the crash found in #1181. Under certain circumstances, the ExtensionService will scan every dll in the executing directory to look for extensions. If an unmanaged dll is encountered at this point, NUnit crashes.

(#1181 was closed as a sub-issue of #1009, although this PR only catches the single case found in #1181 - I haven't done anything further to investigate other cases.)

@CharliePoole
Copy link
Contributor

This looks good to me, with the possible exception that maybe we should log any errors. @rprouse what do you think?

@CharliePoole
Copy link
Contributor

Something else I just thought of... we can get to the code in question in two ways: (1) scanning a directory using a wild-card pattern and (2) because the file was explicitly listed in a .addins file. In the first case, it makes sense to silently ignore the error, but in the second it seems wrong. What do you think @ChrisMaddock and @rprouse ?

@ChrisMaddock
Copy link
Member Author

@CharliePoole Thanks, the second use-case hadn't crossed my mind.

Sounds to me like FindExtensionsInAssembly() needs to know if it's scanning or looking at a specified assembly, and either skip, or throw an NUnitEngineException specifying the invalid file?

@CharliePoole
Copy link
Contributor

That's what it seems to me. If the file was actually listed by name, it's a configuration error.

@ChrisMaddock
Copy link
Member Author

Thanks @CharliePoole - have added that in.

@CharliePoole
Copy link
Contributor

Looks good to me. We need a second review. @rprouse ?

CharliePoole added a commit that referenced this pull request Feb 15, 2016
This being simple enough and passing it's tests, I decided to merge without a second review. Fixes #1181.
@CharliePoole CharliePoole merged commit 19964b4 into nunit:master Feb 15, 2016
@ChrisMaddock ChrisMaddock deleted the issue-1181 branch February 15, 2016 22:00
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