test: refactor to move knowledge for in-process Agent re-use into Agent methods

[trentm]

Move away from using the test/_agent.js wrapper in tests, in favour of using the real Agent class directly and agent.destroy() at the end of a test to clean up. Agent#destroy() has been improved and Instrumentation#stop() added to properly handle agent clean-up (within reason) such that tests can serially create multiple instances of the Agent.

Agent#destroy() and Instrumentation#stop() are mostly what the "test/_agent.js" cleanup was doing with some improvements:

• restore Error.stackTraceLimit
• disable the running asyncHook

Not all usage of test/_agent.js has been removed -- just the major users agents.test.js and config.test.js.
I'll look at doing the others in subsequent changes. This PR is big enough already.

The motivation for this work is to help with test suite updates for the async context work. The internals of Instrumentation are changing, so it has been a pain that the test suite assumes so many internals.

[apmmachine]

💚 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: 2021-08-27T00:26:16.849+0000

• Duration: 19 min 47 sec

• Commit: 6810173

Test stats 🧪

Test	Results
Failed	0
Passed	20
Skipped	0
Total	20

Trends 🧪

[trentm]

Add some reviewer notes.

[trentm]

REVIEWER NOTE: All the changes in this file were to export DEFAULTS to use in one of the tests below. Then, because the CAPTURE_... constants are used in the declaration of DEFAULTS, it was cleaner to declare the exports as consts and setup module.exports at the bottom.

[trentm]

This asyncHook.disable() moved to Instrumentation#stop(), removing the need for _ELASTIC_APM_ASYNC_HOOKS_RESETTABLE.

[trentm]

REVIEWER NOTE: These few support functions were moved up from the bottom of the file without modification. The structure of the file is now:

// imports and consts
// support functions
// tests

[astorm]

The changes here look reasonable and seem like appropriate steps towards being able to better test changes we're making to our context handling.

Approving, pending one discretionary question.

I noticed that a run of test/agent.test.js went from 853 assertions to 572 assertions with these changes.

$ npx tape test/agent.test.js
//...
1..853
# tests 853
# pass  853

# ok

$ git checkout trentm/sans-mock-agent
$ npx tape test/agent.test.js
//...
1..572
# tests 572
# pass  572

# ok

These seemed to be from test suites that used the now removed APMServerWithDefaultAsserts function. Here's one example

Before:

# capture location stack trace - off (param msg)
ok 539 should be a POST request
ok 540 should be sent to the intake endpoint
ok 541 metadata should always be sent first
ok 542 should be strictly equal
ok 543 should be deeply equivalent
ok 544 should be deeply equivalent
ok 545 should be deeply equivalent
ok 546 should be strictly equal
ok 547 should be strictly equal
ok 548 should be strictly equal
ok 549 should be truthy
ok 550 should be strictly equal
ok 551 should have a pid greater than 0
ok 552 should have a process title
ok 553 should be strictly equal
ok 554 should be deeply equivalent
ok 555 should have at least two process arguments
ok 556 should be strictly equal
ok 557 should not have a log.stacktrace
ok 558 should not have an exception

After:

# capture location stack trace - off (param msg)
ok 384 APM server got 2 events
ok 385 should be strictly equal
ok 386 should not have a log.stacktrace
ok 387 should not have an exception

Is there any concern about this lost coverage? Or were the tests that came along with the old APMServerWithDefaultAsserts now redundant?

[astorm]

👍 I did a quick scan of the work done in both the agent's constructor and agent.start() and the values reset here appear to be sufficient.

[astorm]

👍 I generally prefer "export everything once at the bottom" approach, and I never liked relying on the config function being hoisted up

[astorm]

TIL: https://stackoverflow.com/a/37528645/4668

[astorm]

👍 Appears to reset the appropriate properties.

[trentm]

Is there any concern about this lost coverage? Or were the tests that came along with the old APMServerWithDefaultAsserts now redundant?

There is no concern in my humble opinion. Here is what the old test code would do for each of the 33 usages of APMServerWithDefaultAsserts:

function APMServerWithDefaultAsserts (t, agentOpts, mockOpts) {
  var server = APMServer(agentOpts, mockOpts)
    .on('request', validateRequest(t))
    .on('data-metadata', validateMetadata(t))
 // ...
}

function validateRequest (t) {
  return function (req) {
    t.strictEqual(req.method, 'POST', 'should be a POST request')
    t.strictEqual(req.url, '/intake/v2/events', 'should be sent to the intake endpoint')
  }
}

function validateMetadata (t) {
  return function (data, index) {
    t.strictEqual(index, 0, 'metadata should always be sent first')
    assertMetadata(t, data)
  }
}

• The validateRequest asserts are redudant because the MockAPMServer being used now only captures events for tests if the request is a POST /intake/v2/events:

      } else if (req.method === 'POST' && parsedUrl.pathname === '/intake/v2/events') {
        body
          .split(/\n/g) // parse each line
          .filter(line => line.trim()) // ... if it is non-empty
          .forEach(line => {
            this.events.push(JSON.parse(line)) // ... append to this.events
          })

• The assertMetadata is still being done, but just not for every test -- in about 12 of them instead of 33.

[astorm]

@trentm sounds good! Approved x2

[trentm]

Thanks for the review!