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 unexpected IsDotNet behavior for non-dotnet namespaces #1193

Merged
merged 2 commits into from Nov 27, 2019

Conversation

@FLeXyo
Copy link
Contributor

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

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
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

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
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 approved these changes Nov 27, 2019
@jnyrup jnyrup merged commit 88b07d4 into fluentassertions:master Nov 27, 2019
1 check failed
1 check failed
continuous-integration/appveyor/pr AppVeyor build failed
Details
@jnyrup

This comment has been minimized.

Copy link
Collaborator

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
Projects
None yet
3 participants
You can’t perform that action at this time.