Convert tests to snapshot-based tests

[RussKie]

Convert selected tests to the snapshot-based testing to simplify the tests and simplify and improve the test experience - whenever a test fails you won't need to guess where, you'll see it in a diff tool of your choice.
E.g.:

[davidfowl]

How many merge conflicts with existing PRs is this going to cause? Seems like a big notification that we’re changing how we’re going to be doing snapshotting would be required before merging something this significant and disruptive

[RussKie]

How many merge conflicts with existing PRs is this going to cause?

I can't see any reasons why it should cause conflicts... And if there's a conflict it should be simple to solve.

[RussKie]

Seems like a big notification that we’re changing how we’re going to be doing snapshotting would be required before merging something this significant and disruptive

You can continue to do it the old way, if you desire so. It just that way is painful, laborious and hard to maintain.

[davidfowl]

I don’t have any desire to use the old way, but if you make a change this big to how we do baseline testing, there should be a discussion with the engineering team.

I think the this is a positive change but not one you PR when it affects everyone’s workflow. There should be a show and tell

[sebastienros]

1- What does this do? Is it just to tell that .bicep files are text and not binary? What happens without it?

    static AzureContainerAppsTests()
    {
        FileExtensions.AddTextExtension("bicep");
    }

2- Is there a place in the tests projects that it could be put so it doesn't have to be repeated?
3- Could the verified files be in a subfolder of the test, like ./verified ? I have checked but I am worried these files take some space in the VS editor.
4- Should we add a link to the file in a comment for each Verify call, just to have a quick access to what is generated? Or is your experience showing that it's actually easy to find these

I didn't see anything that would break existing PRs, unless the ones touching these bicep results.

[danmoseley]

My suggestion is change this PR into a minimal one (just changing exactly one test to show the pattern) then get consensus in the chat you started. Then if we want to go ahead, it would be best to do it in chunks to minimize merge conflicts/make review feasible..

[RussKie]

1- What does this do? Is it just to tell that .bicep files are text and not binary? What happens without it?

    static AzureContainerAppsTests()
    {
        FileExtensions.AddTextExtension("bicep");
    }

This allows us to have verified files as ".bicep" instead of ".txt". Whilst this isn't strictly speaking necessary, it makes the IDEs to render files with colours.

2- Is there a place in the tests projects that it could be put so it doesn't have to be repeated?

I don't know tbh. There's no guarantee all tests will be run, so this needs to be done at the assembly-level... Maybe ModuleInitializer can help?

3- Could the verified files be in a subfolder of the test, like ./verified ? I have checked but I am worried these files take some space in the VS editor. 4- Should we add a link to the file in a comment for each Verify call, just to have a quick access to what is generated? Or is your experience showing that it's actually easy to find these

The verified files can be placed in custom subfolders, if necessary.
In my experience, it's generally easy to find these files - be the files placed next to the test files or in subfolders. Again, this depends on consistency of organisation - if one test decides to place files under "verified/" folder, another puts those in "../Mock/snapshots", then there will be chaos.
When using subfolders we shoud be cognisant of paths - some verified files can already be very long, and making paths longer can be an (additional) issue (yes, there's of course git config --global core.longpaths true, but nevertheless).

I didn't see anything that would break existing PRs, unless the ones touching these bicep results.

[sebastienros]

This is something that existing PRs will need to update, though easy to do, since the build verifies there isn't any unnecessary using statements. Couldn't find where this implicit one is coming from though.

[RussKie]

Me either, probably somewhere from Verifier or its dependencies.

[RussKie]

So... is there anything blocking this?