Investigate false positive test

[jhnlsn]

windows_migration_core_test.go has a potential for having a false positive test with input2.json and output2.json.

it was found that an empty files object is included when it is expected that one should not be.

See PR: #283

[kyoicito]

I did some debug and investigation against this issue.

Premise

I ran the test with the latest commit: https://github.com/aws/amazon-cloudwatch-agent/commit/e9190ceb9b1a052e1da84d53dfd1b7db1c9a3a3e

Investigation

I put two t.Logf statements within TestMapOldWindowsConfigToNewConfig function for debug. https://github.com/aws/amazon-cloudwatch-agent/blob/e9190ceb9b1a052e1da84d53dfd1b7db1c9a3a3e/tool/processors/migration/windows/windows_migration_core_test.go#L47-L50

                        // Assert
                        if !AreTwoConfigurationsEqual(newConfig, expectedConfig) {
                                t.Errorf("The generated new config is incorrect, got:\n %v\n, want:\n %v.\n", newConfig, expectedConfig)
                        }else{  
                                t.Logf("Generated Config:\n %#v\n", newConfig.Logs.LogsCollected)
                                t.Logf("Expected Config:\n %#v\n", expectedConfig.Logs.LogsCollected)
                        }

Then run the test with the command below.

go test -v -run TestMapOldWindowsConfigToNewConfig ./tool/processors/migration/windows/...

The result is below (just picking up the part of input2.json)

=== RUN   TestMapOldWindowsConfigToNewConfig/testData/input2.json,_testData/output2.json
    windows_migration_core_test.go:51: Generated Config:
         windows.LogsCollectedEntry{Files:windows.FilesEntry{CollectList:[]windows.NewCwConfigLog(nil)}, WindowsEvents:windows.WindowsEventsEntry{CollectList:[]windows.NewCwConfigWindowsEventLog{windows.NewCwConfigWindowsEventLog{EventName:"Application", EventLevels:[]string{"ERROR", "WARNING", "INFORMATION"}, CloudwatchLogGroupName:"Application", CloudwatchLogStreamName:"{instance_id}", EventFormat:"text"}}}}
    windows_migration_core_test.go:52: Expected Config:
         windows.LogsCollectedEntry{Files:windows.FilesEntry{CollectList:[]windows.NewCwConfigLog(nil)}, WindowsEvents:windows.WindowsEventsEntry{CollectList:[]windows.NewCwConfigWindowsEventLog{windows.NewCwConfigWindowsEventLog{EventName:"Application", EventLevels:[]string{"ERROR", "WARNING", "INFORMATION"}, CloudwatchLogGroupName:"Application", CloudwatchLogStreamName:"{instance_id}", EventFormat:"text"}}}}

This means not only newConfig but also expectedConfig also includes empty files object.

Files:windows.FilesEntry{CollectList:[]windows.NewCwConfigLog(nil)}

expectedConfig is generated by ReadNewConfigFromPath function, and it calls json.Unmarshal with NewCwConfig structure. https://github.com/aws/amazon-cloudwatch-agent/blob/e9190ceb9b1a052e1da84d53dfd1b7db1c9a3a3e/tool/processors/migration/windows/windows_migration_core_test.go#L41 https://github.com/aws/amazon-cloudwatch-agent/blob/e9190ceb9b1a052e1da84d53dfd1b7db1c9a3a3e/tool/processors/migration/windows/windows_util.go#L24-L33

The root cause is the test uses the same structure format for comparison. If we want to check the generated config strictly, we should compare them as JSON instead of as NewCwConfig struct.

[jhnlsn]

Really appreciate you looking into this! That would precisely explain the behavior with the test, and I would agree that it would be better if we performed a json string compare instead of marshaling the expected output json.

[SaxyPandaBear]

I poked around on the PR branch and made updates to attempt to do JSON string comparisons. For input/output 2, I get the following strings:

Generated:

{
    "agent": {
        "region": "us-west-2"
    },
    "logs": {
        "logs_collected": {
            "windows_events": {
                "collect_list": [
                    {
                        "event_name": "Application",
                        "event_levels": [
                            "ERROR",
                            "WARNING",
                            "INFORMATION"
                        ],
                        "log_group_name": "Application",
                        "log_stream_name": "{instance_id}",
                        "event_format": "text"
                    }
                ]
            }
        }
    }
}

Expected:

{
    "agent": {
      "region": "us-west-2"
    },
    "logs": {
      "logs_collected": {
        "windows_events": {
          "collect_list": [
            {
              "event_name": "Application",
              "event_levels": [
                "ERROR",
                "WARNING",
                "INFORMATION"
              ],
              "event_format": "text",
              "log_group_name": "Application",
              "log_stream_name": "{instance_id}"
            }
          ]
        }
      }
    }
  }

---

I get the generated config by marshalling the config struct, and got the expected config by adding a new function that reads the file and just returns the file contents as a string, then use assert.JSONEq() to compare. The test still passes, as they are still equivalent JSON blobs