Skip to content

Fix unexpected IsDotNet behavior for non-dotnet namespaces#1193

Merged
jnyrup merged 2 commits intofluentassertions:masterfrom
FLeXyo:fix-isdotnet
Nov 27, 2019
Merged

Fix unexpected IsDotNet behavior for non-dotnet namespaces#1193
jnyrup merged 2 commits intofluentassertions:masterfrom
FLeXyo:fix-isdotnet

Conversation

@FLeXyo
Copy link
Copy Markdown
Contributor

@FLeXyo FLeXyo commented Nov 26, 2019

The check in IsDotNet only checks if the namespace starts with "System" which can cause non-dotnet namespaces like SystemSomething.RestApi to return true unexpectedly.

I struggled with this one at work today for a few hours as our product name (and therefor our namespaces) are SystemProductName.RestApi. I noticed it because the variable names were missing from our failing asserts even when building in debug mode.

IMPORTANT

  • The code complies with the Coding Guidelines for C#.
  • The changes are covered by a new or existing set of unit tests which follow the Arrange-Act-Assert syntax such as is used in this example.
  • If the contribution affects the documentation, please include your changes to documentation.md in this pull request so the documentation will appear on the website.

@jnyrup
Copy link
Copy Markdown
Member

jnyrup commented Nov 26, 2019

Thanks for pointing this out and submitting a PR.
Would you be able to create a unit test to verify the bug and the fix?

Side note: Less than three weeks ago the same flawed logic was discovered for ThatAreUnderNamespace

@FLeXyo
Copy link
Copy Markdown
Contributor Author

FLeXyo commented Nov 26, 2019

@jnyrup sure I'll give it a try. Any suggestions on where to write the unit tests?

@jnyrup
Copy link
Copy Markdown
Member

jnyrup commented Nov 26, 2019

We don't seem to have tests targeting CallerIdentifier directly, so feel free to create a new test class named CallerIdentifierSpecs.cs.

@FLeXyo
Copy link
Copy Markdown
Contributor Author

FLeXyo commented Nov 27, 2019

Hopefully this does it, but I have my doubts :) I'm quite new to unit testing as well as this library. Testing the thing that you use to test certainly makes it a bit more confusing 🥴

No idea why the appveyor build fails as it works fine for me locally with build.ps1.

@jnyrup jnyrup merged commit 88b07d4 into fluentassertions:master Nov 27, 2019
@jnyrup
Copy link
Copy Markdown
Member

jnyrup commented Nov 27, 2019

@FLeXyo Thanks for fixing this!
The failing tests are a recurring issue #1085

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