Convert ASP.NET tests to use Snapshot Testing with VerifyTests/Verify by andrewlock · Pull Request #1454 · DataDog/dd-trace-dotnet

andrewlock · 2021-05-07T08:37:32Z

Note: The +/- line count is deceiving. If you exclude the snapshot files, there is a massive drop in lines of code, and an even bigger drop in number of characters 😄 e.g. this file is gone!

This initial PoC creates some helpers for verifying MockTracerAgent.Spans and converts our existing ASP.NET integration tests to use snapshot testing.

Challenges:

~~Needed to write a custom scrubber for token-replacement of IDs. This is pretty crude atm but it works. May be enabled OOTB in future Verify versions~~ Done in Scrub numeric ids VerifyTests/Verify#351 and Add support for scrubbing nullable numeric IDs VerifyTests/Verify#352!
Verify doesn't support < .NET 4.6.1. That may mean it's not a viable option for every case given we need to support .NET 4.5.2 🙁
Ran into some weird errors due to trying to create filenames with / in them. Solved by stripping the /.
Multiple test classes in a file requires a bit of extra verbosity that won't be required in most cases
Running integration tests locally is still a royal pain ATM, which makes generating the initial verification text a bit of a pain. Obviously we should be able to do this though, and Nuke will help 😉
Had to do a bit of csproj fiddling to get the nesting to work, again because we have multiple test classes in a file in this case. Overall it works well though:
Different architectures apparently have different stack traces, so need to use different snapshots per architecture. We could just strip out the error entirely, but if this is stable, would be a better option I think. Native support should be coming soon to Verify.

Wins:

All the benefits of snapshot testing - easier to understand, tests more things, easier to understand failures
Easily plugs into existing tests by just replacing all the asserts with a single Verify method
Personally think this is a great option for bread-and-butter integration testing. Makes visualising traces a lot easier, and gives more complete coverage

Remaining questions:

~~How stable are these spans across platforms?~~ well integration tests work, so there we go!
~~Need to make the environments deterministic - just noticed my system level DD_ENV=andrew has made its way into the span!~~ Added explicit setting of DD_ENV for sample projects in 5c93c35. Can be overwritten, but should ensure consistency in other environments.
There appears to be an issue with the solution path scrubbing, which is causing the current failures in the integration tests pipeline. The attributes don't appear to be embedded in the integration test project for some reason. This is probably something to do with our convoluted CI build process (as it works in VS) but I'm not sure why... 🤞 Fixed in de6965c, it's because $(SolutionDir) isn't set when calling dotnet build on a project as opposed to the solution.
Need to check how Verify handles reordering in lists/dictionaries

zacharycmontoya · 2021-05-25T22:25:11Z

+
+            // Overriding the type name here as we have multiple test classes in the file
+            // Ensures that we get nice file nesting in Solution Explorer
+            await Verifier.Verify(spans, settings)


Can we also set up Verify to place all snapshots in its own directory? I'm thinking something like /test/snapshots/<ProjectName>/**/verified.txt

Well, we could yes. And for this example, that might make sense because there are so many variations, but in general I think the "default" behaviour is preferable. Having the snapshots stored "with" the test itself makes it easier to manually check them and for referring back and forth I think. We shouldn't have to do all this customisation in most cases.

I'm open to what people think though - it looks messier in file browsers, but in VS/Rider, having the snapshots folded under your test class is preferable I think, as you can easily jump to individual snapshots, but they're hidden by default:

zacharycmontoya · 2021-05-25T22:26:23Z

        }

+        public static TheoryData<string, int> Data() => new()


This is so nice, we just need to specify the inputs and ensure the output is expected 😍

zacharycmontoya

LGTM. Let's verify all the things!

tonyredondo

LGTM nice!

lucaspimentel

Big step forward for our testing suite, thanks! Left a couple of non-blocking comments.

lucaspimentel · 2021-05-26T13:20:32Z


-        public AspNetWebApi2Tests(IisFixture iisFixture, ITestOutputHelper output, bool enableCallTarget, bool classicMode)
+        public AspNetWebApi2Tests(IisFixture iisFixture, ITestOutputHelper output, bool enableCallTarget, bool classicMode, bool enableFeatureFlag)


Consider renaming parameter enableFeatureFlag to something more obvious (which feature flag?). Something like enableRouteTemplateResourceNames (but shorter?). Thanks!

done in 20bec17

lucaspimentel · 2021-05-26T13:26:02Z

+                      + (enableCallTarget ? ".CallSite" : ".CallTarget")
+                      + (classicMode ? ".Classic" : ".Integrated")
+                      + (enableFeatureFlag ? ".NoFF" : ".WithFF")
+                      + (RuntimeInformation.ProcessArchitecture == Architecture.X64 ? ".X64" : ".X86"); // assume that arm is the same


Should we include ARM support now so we don't have to come back and fix later?
https://docs.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.architecture

I'm in two minds. Will the call stack be different on Arm? Maybe not? Might not be necessary? I'm inclined to punt until we do run this on ARM (Windows Arm, I guess it's just a matter of time?)

These should never be checked in (in contrast to *.verified.* files

Json.NET update required as Verify depends on 13.0.1

Lets us use C# 9 Module Initializers on < .NET 5

We may need to add additional scrubbers etc. In addition to the built-in scrubbers, I'm - Excluding the variable duration/start fields - Fixing localhost ports to all be :0000 - Fixing the _dd.tracer_kr: 1.0 Also adds helper for fetching the web spans (without asserting), and for sanitising paths for use in filenames

The stack traces are different depending on architecture. There's a PR in to Verify to support this OOTB: VerifyTests/Verify#367

andrewlock force-pushed the andrew/verify branch from 632365c to e8c5d72 Compare May 20, 2021 10:51

andrewlock marked this pull request as ready for review May 21, 2021 11:45

andrewlock requested a review from a team as a code owner May 21, 2021 11:45

andrewlock changed the title ~~Initial PoC for switching to VerifyTests/Verify~~ Convert ASP.NET tests to use VerifyTests/Verify May 21, 2021

andrewlock changed the title ~~Convert ASP.NET tests to use VerifyTests/Verify~~ Convert ASP.NET tests to use Snapshot Testing with VerifyTests/Verify May 21, 2021

andrewlock force-pushed the andrew/verify branch 5 times, most recently from d310573 to af90959 Compare May 25, 2021 13:41

zacharycmontoya reviewed May 25, 2021

View reviewed changes

zacharycmontoya approved these changes May 25, 2021

View reviewed changes

tonyredondo approved these changes May 26, 2021

View reviewed changes

lucaspimentel approved these changes May 26, 2021

View reviewed changes

lucaspimentel added the area:tests unit tests, integration tests label May 26, 2021

andrewlock added 14 commits May 26, 2021 15:11

Add required gitignore for Verify received files

41a10d9

These should never be checked in (in contrast to *.verified.* files

Install Verify and update Newtonsoft Json

4a97f8e

Json.NET update required as Verify depends on 13.0.1

Add ModuleInitializerAttribute for < .NET 5

9c4673c

Lets us use C# 9 Module Initializers on < .NET 5

Ensure sample apps have a consistent environment across machines

9e454a4

Populate $(SolutionDir) property if not already set

2ddc3eb

Convert AspNetMvc4 tests to use Snapshot testing

785a217

Convert AspNetMvc5 tests to use Snapshot testing

f268f5f

Convert AspNetWebApi2 tests to use Snapshot testing

dba427c

Include architecture in the file name for aspnetcore tests

799bea0

The stack traces are different depending on architecture. There's a PR in to Verify to support this OOTB: VerifyTests/Verify#367

Add x64 Snapshots

03d4587

Add x86 Snapshots

56de9cc

Add copyright headers

2f6aacd

Fix CI app tests

1d1475a

Rename enableFeatureFlag to enableRouteTemplateResourceNames

20bec17

andrewlock force-pushed the andrew/verify branch from af90959 to 20bec17 Compare May 26, 2021 14:13

andrewlock merged commit c06a119 into master May 26, 2021

andrewlock deleted the andrew/verify branch May 26, 2021 15:10

pellared mentioned this pull request May 31, 2021

Upstream sync 2021-05-31 open-telemetry/opentelemetry-dotnet-instrumentation#188

Merged

andrewlock added this to the 1.27.0 milestone Jun 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Convert ASP.NET tests to use Snapshot Testing with VerifyTests/Verify#1454

Convert ASP.NET tests to use Snapshot Testing with VerifyTests/Verify#1454
andrewlock merged 15 commits into
masterfrom
andrew/verify

andrewlock commented May 7, 2021 •

edited

Loading

Uh oh!

zacharycmontoya May 25, 2021

Uh oh!

andrewlock May 26, 2021 •

edited

Loading

Uh oh!

zacharycmontoya May 25, 2021

Uh oh!

zacharycmontoya left a comment

Uh oh!

tonyredondo left a comment

Uh oh!

lucaspimentel left a comment

Uh oh!

lucaspimentel May 26, 2021

Uh oh!

andrewlock May 26, 2021

Uh oh!

lucaspimentel May 26, 2021

Uh oh!

andrewlock May 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants


		public AspNetWebApi2Tests(IisFixture iisFixture, ITestOutputHelper output, bool enableCallTarget, bool classicMode)
		public AspNetWebApi2Tests(IisFixture iisFixture, ITestOutputHelper output, bool enableCallTarget, bool classicMode, bool enableFeatureFlag)

Uh oh!

Conversation

andrewlock commented May 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zacharycmontoya May 25, 2021

Choose a reason for hiding this comment

Uh oh!

andrewlock May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zacharycmontoya May 25, 2021

Choose a reason for hiding this comment

Uh oh!

zacharycmontoya left a comment

Choose a reason for hiding this comment

Uh oh!

tonyredondo left a comment

Choose a reason for hiding this comment

Uh oh!

lucaspimentel left a comment

Choose a reason for hiding this comment

Uh oh!

lucaspimentel May 26, 2021

Choose a reason for hiding this comment

Uh oh!

andrewlock May 26, 2021

Choose a reason for hiding this comment

Uh oh!

lucaspimentel May 26, 2021

Choose a reason for hiding this comment

Uh oh!

andrewlock May 26, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

andrewlock commented May 7, 2021 •

edited

Loading

andrewlock May 26, 2021 •

edited

Loading