Structure main_test using the Testify Suite package

[jlvoiseux]

Motivation / Summary

While the addition of unit tests for main.go have been very useful to implement the backoff mechanism (#148) and assess the behaviour of the extension in several edge cases (#136), reliance on package-scoped state variables such as ApmServerTransportState induces hard-to-detect race conditions.

An example is the commit c6b3fb5, whose CI pipeline failed due to a race between the goroutine tasked with setting the ApmServerTransportState back to pending and the start of TestFullChannel. The latter started before the goroutine completed its execution, and none of the events that we supposed to be queued during the test were sent to the APM Server. The issue did not appear in the many commits merged beforehand, with the very same test setup.

In an attempt to prevent those races, this PR refactors main_test.go by using the suite package of testify. This package allows for the definition of a SetupTest() function, that we can use to reset useful variables before each test, hence greatly reducing the risks of unexpected behavior.

Files changed

All changes are contained in main_test.go.

How to test

• Clone the repository
• Go to the extension folder: cd apm-lambda-extension
• Run the unit tests: go test ./.... All tests should pass (with the exception of e2e_test, which should be skipped).

ok      elastic/apm-lambda-extension    21.122s
ok      elastic/apm-lambda-extension/e2e-testing        0.463s
ok      elastic/apm-lambda-extension/extension  2.880s
ok      elastic/apm-lambda-extension/logsapi    0.625s

[ghost]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2022-04-07T14:10:51.943+0000

• Duration: 7 min 18 sec

Test stats 🧪

Test	Results
Failed	0
Passed	164
Skipped	4
Total	168

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

• /test : Re-trigger the build.

• run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

[axw]

Looks good.

We're still using package-level state though right? MainUnitTestsSuite.SetUpTest still calls extension.SetApmServerTransportState underneath the covers, so the underlying issue still remains?

It doesn't have to be done now, but I think we should try to get rid of the package-level var by introducing an "APMServer" struct type which contains all of the state, and have the extension instantiate one of those and pass it around as needed. Then the tests can create their own instances. This would have the additional benefit of being able to run tests in parallel.

Also, as an alternative to having a testify suite which sets up all the things, consider making smaller, more targeted functions which use testing.F.Cleanup to clean up on test completion.

e.g.

func newMockLambdaServer(t testing.TB) *http.Server {
    lambdaServer := ...
    t.Cleanup(lambdaServer.Close)
    return lambdaServer
}

func newMockAPMServer(t testing.TB) (*http.Server, *apmServerInternals) {
    ...
}

[jlvoiseux]

@axw Thanks for the review! I will merge this following the implementation of TearDownTest (c102527), and open two issues:

• One dedicated to the replacement of Package variables by structs, instantiated as needed by the tests
• Make the main-unit-tests more modular by using test.F.Cleanup