Newrelic 3123/esm test loader #141

mrickard · 2022-09-09T16:57:34Z

Proposed Release Notes

Added the capacity of running the ESM loader ESM module tests using this project's test runner.

Links

https://issues.newrelic.com/browse/NEWRELIC-3123

Details

This change requires the default ESM loader from newrelic/node-newrelic#1348, or an equivalent ESM loader defined as a path from the agent project root as the value of process.env.NR_LOADER.

Signed-off-by: mrickard <[email protected]>

…dules Signed-off-by: mrickard <[email protected]>

bizob2828

great start!

lib/versioned/test.js

lib/versioned/runner.js

…efined in process.env.loader Signed-off-by: mrickard <[email protected]>

bizob2828 · 2022-09-12T21:07:08Z

lib/versioned/runner.js

+  if (process.env.packageType && process.env.packageType === 'module') {
+    // The default loader is in the agent root, so this uses process.cwd() instead of __directory.
+    const cwd = process.cwd().slice(0, process.cwd().indexOf('/test/'))
+    let loaderPath = path.resolve(`${cwd}/`, 'esm-loader.mjs')


I'm not sure there's a time we'd use the agent loader. At the very least it'd be a test loader. In my branch i called it test/lib/test-loader.mjs. I was going to explore using testdouble to mock the agent API but regardless you'd need to call a loader and if we used testdouble we wouldn't be able to call our loader with that so we'd have to chain them which is fine

To avoid having to pass in the test loader I see we have a few options here:

set the default to the agent's test/lib/test-loader.mjs

move the test loader into the versioned repo and reference that as the default

I verified passing in env var works:

loader=./test/lib/test-loader.mjs npm run versioned express-esm

It makes sense to use the test loader. Default to test/lib/test-loader.mjs is fine, I'd think.

bizob2828 · 2022-09-12T21:11:15Z

lib/versioned/runner.js

+    // The default loader is in the agent root, so this uses process.cwd() instead of __directory.
+    const cwd = process.cwd().slice(0, process.cwd().indexOf('/test/'))
+    let loaderPath = path.resolve(`${cwd}/`, 'esm-loader.mjs')
+    if (process.env.loader) {


maybe make this something that could be less of a conflict because technically this is opened source.

NR_LOADER?

Do we prefer the more-terse NR_ or something like NEW_RELIC_LOADER? (Personally, I'd prefer terse.)

sounds good to me

Signed-off-by: mrickard <[email protected]>

bizob2828

I'm just thinking through test coverage now and put a few thoughts down

bizob2828 · 2022-09-13T14:17:38Z

lib/versioned/test.js

@@ -80,6 +80,7 @@ function Test(directory, pkgVersions, opts = {}) {
  this.duration = 0
  this.allPkgs = !!allPkgs
  this.strict = !!opts.strict
+  this.type = pkg.type


we have unit test for this file, might be good to add some assertions around type.

bizob2828 · 2022-09-13T14:19:14Z

lib/versioned/runner.js

@@ -97,7 +98,19 @@ function installPackages(cb) {
 function runTests(cb) {
  // TODO: Add tap arguments, such as color.
  process.send({ status: 'running' })
-  spawn('node', [testFile], function testHandler(err) {
+  let args = [testFile]
+  if (process.env.packageType && process.env.packageType === 'module') {


thinking more of this we could just default the type in lib/versioned/test.js to commonjs

I initially did do that--it's easy enough to restore!

bizob2828 · 2022-09-13T14:21:34Z

lib/versioned/runner.js

+  let args = [testFile]
+  if (process.env.packageType && process.env.packageType === 'module') {
+    // The test loader is defined in the agent, so this uses process.cwd() instead of __directory.
+    const cwd = process.cwd().slice(0, process.cwd().indexOf('/test/'))


it's unfortunate we've designed a runner that is very hard to unit test. but i confirmed this works manually. but thinking more of this if we shift all this logic to the test we could test it and hten just pass the loader in the spawn, thoughts?

Thinking this through...this code is only necessary here to get the directory of the agent running it. It's kind of awkward, and I agree that would be better handled by the agent.

If we added it in the agent code as an environment variable passed to the spawn there, I'd think we could just rely on the loader path env var.

I'm not sure I understand what you're saying. I was just thinking of moving this logic from lib/versioned/runner to lib/versioned/test so we could test the logic.

I was reading that as a recommendation to move this code entirely out of runner and into the node agent. I can agree with adding it to this repos tests to test the logic...though the runner is still going to need to know where to find a loader when this runner is kicking off versioned tests, so some version of pathway detection will still need to be here. (Though if we're bundling a test loader in this repo, we could use __dirname instead of the path manipulation here.)

Signed-off-by: mrickard <[email protected]>

bizob2828 · 2022-09-14T18:52:21Z

lib/versioned/runner.js

@@ -99,7 +99,7 @@ function runTests(cb) {
  // TODO: Add tap arguments, such as color.
  process.send({ status: 'running' })
  let args = [testFile]
-  if (process.env.NR_LOADER) {
+  if (process.env.nr_esm_loader) {
    const loaderPath = path.resolve(process.env.NR_LOADER)


this is the wrong env var now. should we unset this env var after every run so subsequent suites will not inherit?

I'd agree that we should unset if we want to be sure.

I'd think this should be failing--there's a disjuncture between the env vars on 102 and 103.

this is out of date. it's been updated

bizob2828

Verified on full runs of versioned tests that have both esm and cjs tests. Great work! Can you start preparing a minor release after the CI on main passes so we can get this updated in main agent?

bizob2828

sorry i broke the build. committed the missing file

mrickard added 2 commits September 9, 2022 12:53

Added JetBrains config to .gitignore

cada6a6

Signed-off-by: mrickard <[email protected]>

Get dependency type from package.json and use alternate loader for mo…

9de8e84

…dules Signed-off-by: mrickard <[email protected]>

bizob2828 reviewed Sep 9, 2022

View reviewed changes

lib/versioned/test.js Outdated Show resolved Hide resolved

lib/versioned/runner.js Outdated Show resolved Hide resolved

Corrected loader argument to cp/.spawn and allowed for loader to be d…

982da86

…efined in process.env.loader Signed-off-by: mrickard <[email protected]>

mrickard marked this pull request as ready for review September 12, 2022 19:52

mrickard requested a review from bizob2828 September 12, 2022 20:05

bizob2828 reviewed Sep 12, 2022

View reviewed changes

Updated to use test loader, and renamed loader env var to NR_LOADER

69a7cc2

Signed-off-by: mrickard <[email protected]>

bizob2828 reviewed Sep 13, 2022

View reviewed changes

bizob2828 mentioned this pull request Sep 13, 2022

Express ESM versioned tests newrelic/node-newrelic#1349

Merged

mrickard and others added 2 commits September 14, 2022 13:46

Refactored to supply loader from test.js, before spawn

334a69a

Signed-off-by: mrickard <[email protected]>

added unit tests for the loader logic in lib/versioned/test.js

8fa5bea

bizob2828 reviewed Sep 14, 2022

View reviewed changes

cleaned up logic in runner for determining when to use loader

683f724

bizob2828 previously approved these changes Sep 14, 2022

View reviewed changes

restored a file i removed

d857dd7

bizob2828 dismissed their stale review via d857dd7 September 14, 2022 19:57

bizob2828 approved these changes Sep 14, 2022

View reviewed changes

mrickard merged commit 7c3e11a into newrelic:main Sep 14, 2022

github-actions bot mentioned this pull request Sep 14, 2022

Release v7.1.0 #142

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Newrelic 3123/esm test loader #141

Newrelic 3123/esm test loader #141

mrickard commented Sep 9, 2022 •

edited

Loading

bizob2828 left a comment

bizob2828 Sep 12, 2022

bizob2828 Sep 12, 2022

bizob2828 Sep 12, 2022

mrickard Sep 12, 2022

bizob2828 Sep 12, 2022

mrickard Sep 12, 2022

bizob2828 Sep 12, 2022

bizob2828 left a comment

bizob2828 Sep 13, 2022

bizob2828 Sep 13, 2022

mrickard Sep 13, 2022

bizob2828 Sep 13, 2022

mrickard Sep 13, 2022

bizob2828 Sep 13, 2022

mrickard Sep 13, 2022

bizob2828 Sep 14, 2022

mrickard Sep 14, 2022

mrickard Sep 14, 2022

bizob2828 Sep 14, 2022

bizob2828 left a comment

bizob2828 left a comment

Newrelic 3123/esm test loader #141

Newrelic 3123/esm test loader #141

Conversation

mrickard commented Sep 9, 2022 • edited Loading

Proposed Release Notes

Links

Details

bizob2828 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bizob2828 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bizob2828 left a comment

Choose a reason for hiding this comment

bizob2828 left a comment

Choose a reason for hiding this comment

mrickard commented Sep 9, 2022 •

edited

Loading