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

Refactor unit tests: data driven tests and leverage Microsoft.CodeAnalysis.Testing patterns #76

Merged
merged 16 commits into from
Jun 10, 2024

Conversation

MattKotsenas
Copy link
Collaborator

@MattKotsenas MattKotsenas commented Jun 10, 2024

After the switch to using Microsoft.CodeAnalysis.Testing, refactor the tests to better follow the test infrastructure conventions. (For additional examples, check out the dotnet/roslyn-analyzers repo).

  1. Use helper classes instead of inheritance to make the tests more flexible
  2. Centralize test defaults in a new Test class rather than in a base class
  3. Add a set of global usings to the default test to simplify tests and make test authoring easier
  4. Use xUnit's data-driven test method to simplify test scenarios
    • Allows for a matrix of tests, both in the global namespace (fixes Refactor tests to cover both global and namespace uses #56) and in a given namespace
    • Makes test debugging a bit easier as there's only a single Moq call asserted in each test
    • Makes reasoning about the test a bit more difficult, as now there's distance between the test clause and the setup
  5. After the test refactoring, there were a few cases of clear duplication; removed and verified code coverage numbers are the same

I tried to also create "doppelganger" tests in this PR, like in NoConstructorArgumentsForInterfaceMockAnalyzerTests where a user creates a Mock<T> class in the namespace to make sure the analyzers are doing fully-qualified name resolution. However, to do that generically requires duplicating a lot of more of Moq's own API, and I'm not sure if that's a good idea, so filed #75 to track.

There's probably more that can be done to DRY out the test templates, but I wanted to make sure this was a good step forward before going any further.

@rjmurillo rjmurillo self-assigned this Jun 10, 2024
@rjmurillo rjmurillo added this to the vNext milestone Jun 10, 2024
rjmurillo
rjmurillo previously approved these changes Jun 10, 2024
@rjmurillo
Copy link
Owner

This generally looks good. Some new warnings in the build log (and when I run locally). Example: https://github.com/rjmurillo/moq.analyzers/actions/runs/9443378573/job/26007024628#step:7:52

[xUnit.net 00:00:00.20] Moq.Analyzers.Test: Skipping test case with duplicate ID 'cdd709071d8219c53260459ad2fb1faa9876c03b' ('Moq.Analyzers.Test.ConstructorArgumentsShouldMatchAnalyzerTests.ShouldAnalyzeConstructorArguments(namespace: "", mock: "new Mock<Foo>(MockBehavior.Default);")' and 'Moq.Analyzers.Test.ConstructorArgumentsShouldMatchAnalyzerTests.ShouldAnalyzeConstructorArguments(namespace: "", mock: "new Mock<Foo>(MockBehavior.Default);")')

xUnit believes they are the same test, but it's actually two separate tests for different analyzers. You want to live with this for now and see if the uber combined test is enough?

@MattKotsenas
Copy link
Collaborator Author

MattKotsenas commented Jun 10, 2024

Thanks for flagging the duplicate test thing. I don't understand what's happening there, so will dig in.

Edit: These are all legit duplicated test cases that I removed. Following the trail backwards, there were some old tests that had cases like:

var mock1 = new Mock<Foo>();
var mock2 = new Mock<FullyQualifiedNamespace.Foo>()

In my other refactorings these got simplified to

var mock1 = new Mock<Foo>();
var mock2 = new Mock<Foo>();

which when turned into test-driven tests xUnit can flag as duplicates. I went back and verified that code coverage metrics don't meaningfully change (slightly complicated by the warning refactor, but withing 4 lines of before).

So I think this is good to do, doesn't regress code coverage, etc.

@rjmurillo rjmurillo merged commit 9a3e801 into rjmurillo:main Jun 10, 2024
4 checks passed
@MattKotsenas MattKotsenas deleted the refactor/lean-tests branch October 23, 2024 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor tests to cover both global and namespace uses
2 participants