Moving to a third party tool for diff

[prateek2408]

Currently TFV test logs are hard to parse because the diff is output as a giant object. It would be nice to reduce the noice here and assist the developer in pinpointing the exact issues.

If this PR is for Terraform, I acknowledge that I have:

• [X] Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
• [X] Generated Terraform, and ran make test and make lint to ensure it passes unit and linter tests.
• [X] Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
• [X] Ran relevant acceptance tests (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
• [X] Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

[modular-magician]

Hello! I am a robot who works on Magic Modules PRs.

I've detected that you're a community contributor. @megan07, a repository maintainer, has been assigned to assist you and help review your changes.

:question: First time contributing? Click here for more details

---

Your assigned reviewer will help review your code by:

• Ensuring it's backwards compatible, covers common error cases, etc.
• Summarizing the change into a user-facing changelog note.
• Passes tests, either our "VCR" suite, a set of presubmit tests, or with manual test runs.

You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails.

If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox.

---

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

TF Validator: Diff ( 3 files changed, 23 insertions(+), 2 deletions(-))

[modular-magician]

Tests analytics

Total tests: 2053 Passed tests 1824 Skipped tests: 226 Failed tests: 3

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccFirebaserulesRelease_BasicRelease|TestAccContainerCluster_withAddons|TestAccContainerCluster_withConfidentialNodes

[modular-magician]

Tests passed during RECORDING mode: TestAccFirebaserulesRelease_BasicRelease[view]

Tests failed during RECORDING mode: TestAccContainerCluster_withAddons[view] TestAccContainerCluster_withConfidentialNodes[view]

Please fix these to complete your PR View the build log or the debug log for each test

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

TF Validator: Diff ( 5 files changed, 33 insertions(+), 8 deletions(-))

[modular-magician]

Tests analytics

Total tests: 2053 Passed tests 1824 Skipped tests: 226 Failed tests: 3

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccFirebaserulesRelease_BasicRelease|TestAccContainerCluster_withConfidentialNodes|TestAccContainerCluster_withAddons

[modular-magician]

Tests passed during RECORDING mode: TestAccFirebaserulesRelease_BasicRelease[view]

Tests failed during RECORDING mode: TestAccContainerCluster_withAddons[view] TestAccContainerCluster_withConfidentialNodes[view]

Please fix these to complete your PR View the build log or the debug log for each test

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

TF Validator: Diff ( 5 files changed, 25 insertions(+), 10 deletions(-))

[prateek2408]

It looks like you will need to update calls to getDiff to instead call assertAssetsMatch - I think that will also mean that you can remove the import of github.com/stretchr/testify/assert for those files.

Rebase of branch diff_tool to main completed. Lets see if the test cases pass now.

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

TF Validator: Diff ( 5 files changed, 28 insertions(+), 13 deletions(-))

[modular-magician]

Tests analytics

Total tests: 2054 Passed tests 1825 Skipped tests: 226 Failed tests: 3

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccFirebaserulesRelease_BasicRelease|TestAccContainerCluster_withConfidentialNodes|TestAccContainerCluster_withAddons

[modular-magician]

Tests passed during RECORDING mode: TestAccFirebaserulesRelease_BasicRelease[view]

Tests failed during RECORDING mode: TestAccContainerCluster_withAddons[view] TestAccContainerCluster_withConfidentialNodes[view]

Please fix these to complete your PR View the build log or the debug log for each test

[prateek2408]

It looks like the tests are passing here 👍 I'm still having issues getting them to pass for me locally, but as long as they're passing here and in the downstream PR (which will need to be made to update go.mod) that is fine with me.

One thing I've noticed is that the error messages are a little hard to understand. For example, right now I'm getting the error locally:

            utils_test.go:238: [Error] The following changes have occurred
            utils_test.go:240:         Element [0 Ancestors 0] has been created from `<nil>` to  `projects/my-project-12345`
            utils_test.go:240:         Element [0 Ancestors 1] has been created from `<nil>` to  `folders/12345`
            utils_test.go:240:         Element [0 Ancestors 2] has been created from `<nil>` to  `organizations/67890`

This is a little hard to understand because (for example) it's not clear which element is element 0 when we're comparing without ordering. Ideally we would be able to see these changes in context - for example:

 Differences in element 0 (+++ added, ---deleted):
       {
           Name: //api-name.googleapis.com/project/my-project-12345/resourceType/resource-id
           Ancestors:
+++        projects/my-project-12345
+++        folders/12345
+++        organizations/67890
           OtherField: Value
       }

This is just an example of what it could look like. The important thing would be having the display of unchanged vs changed lines so you can see the diff in context. Hopefully there is a library that would do the hard lifting for you.

True, going through the code and trying to modify the logic to make the diff look more information

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

TF Validator: Diff ( 5 files changed, 25 insertions(+), 10 deletions(-))

[prateek2408]

I looked at the r3labs/diff package which we use and it seems there in terms of type there can only be 3 values "create, delete, update". So I refactored the logic to have a switch case instead and display different kind of log for each type

So now If I take the previous example then this(Old) output is not changed to (New)-

Old-

            utils_test.go:238: [Error] The following changes have occurred
            utils_test.go:240:         Element [0 Ancestors 0] has been created from `<nil>` to  `projects/my-project-12345`
            utils_test.go:240:         Element [0 Ancestors 1] has been created from `<nil>` to  `folders/12345`
            utils_test.go:240:         Element [0 Ancestors 2] has been created from `<nil>` to  `organizations/67890`

New-

            utils_test.go:238: [Error] Differences Summary (+++ added,  -+ updated, --- deleted):
            utils_test.go:240:         Object [0 Ancestors 0] 
            utils_test.go:243:         +++ projects/my-project-12345
            utils_test.go:240:         Object [0 Ancestors 1] 
            utils_test.go:243:         +++ folders/12345
            utils_test.go:240:         Object [0 Ancestors 2] 
            utils_test.go:243:         +++ organizations/67890

Since we don't have the Name coming to the function so that is not getting displayed. Does this make sense ?

[melinath]

I think that is a lot cleaner, but it does still have the problem of not being in the context of the larger object. Since the function has access to actualAssets and expectedAssets, it seems like you probably have access to the full objects - but at that point you'd practically need to reimplement the diff logic... unless...

maybe you could do something like:

d, err := diff.NewDiffer(diff.SliceOrdering(false))
changes, _ := d.Diff(actualAssets, expectedAssets)
patchlog := diff.Patch(changes, &expectedAssets)
for actualAsset, index in range actualAssets {
    assert.Equal(actualAsset, expectedAssets[index])
}

This is probably not actually exactly how it would work (if it would work at all - I'm a little concerned about all the warnings in the patch function docs) - but the basic idea would be to use the diff to get a "reordered" list of assets that you can more directly compare against.

Alternately, since we know that the assets should have the same Name field, you could iterate through one list, then find the asset with the matching Name in the other list and assert that they're equal. You would just need to be sure to handle the case where an element appeared in one list but not the other.