Visitar URL original
Fix unexpected IsDotNet behavior for non-dotnet namespaces by FLeXyo · Pull Request #1193 · fluentassertions/fluentassertions · GitHub
Skip to content

Fix unexpected IsDotNet behavior for non-dotnet namespaces - #1193

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

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

Conversation

@FLeXyo

@FLeXyo FLeXyo commented Nov 26, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

jnyrup commented Nov 26, 2019

Copy link
Copy Markdown
Member

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

FLeXyo commented Nov 26, 2019

Copy link
Copy Markdown
Contributor Author

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

@jnyrup

jnyrup commented Nov 26, 2019

Copy link
Copy Markdown
Member

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

@FLeXyo

FLeXyo commented Nov 27, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

jnyrup commented Nov 27, 2019

Copy link
Copy Markdown
Member

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

@jnyrup jnyrup mentioned this pull request Nov 30, 2019
Closed
@jnyrup jnyrup mentioned this pull request Oct 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dennisdoomen dennisdoomen dennisdoomen approved these changes
@jnyrup jnyrup jnyrup approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@FLeXyo @jnyrup @dennisdoomen