feat(test-utils): runTestFixture utility for running out-of-process tests #1735
Conversation
…ests Running scripts out-of-process for testing allows running tests with different node options and with isolation. The former is useful for ESM testing. This change includes adding an ESM test for ioredis. This depends on open-telemetry#1694 to pass. Closes: open-telemetry#1731
github-actions
bot
assigned blumamir, dyladan, legendecas, naseemkullah and pichlermarc
github-actions
bot
requested review from
blumamir,
dyladan,
legendecas,
naseemkullah and
pichlermarc
|
The first commit does not include the fix from #1694 for ioredis ESM support, so the test fails: |
|
The second commit adds the fix from #1694 to show that the test now passes: |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1735 +/- ##
=======================================
Coverage 91.42% 91.42%
=======================================
Files 143 143
Lines 7303 7303
Branches 1461 1461
=======================================
Hits 6677 6677
Misses 626 626 |
|
Thanks for working on this; having your help on the topic is very appreciated. 🚀
It should be possible to add
I wonder if we should just do a simple manual tracing setup without
This is amazing; I can see this possibly being very useful for integration testing. I'm wondering though if we may be able to get away with something a bit simpler. Over at #1731 I added a comment that I tried running the existing tests in ESM tests, and was almost successful (% some trouble with iitm+mocha, which may be resolvable). This would basically take care of the telemetry testing with the same coverage as CJS, while out-of-process testing could catch some regressions that would be considered |
That was the part I was missing, thanks.
I don't have strong opinions here yet. Some reasons basing on
I definitely agree that fewer moving parts should be more helpful. And if your other idea pans out, then testing ESM in-process means that using an
Sounds good. |
|
@pichlermarc I've moved the test utility code over the test-utils package now, so it should be clearer what the actual usage would look like. I'm still totally fine waiting on the outcome of your other option on #1731. |
| @@ -51,13 +51,13 @@ const CONFIG = { | |||
| port: parseInt(process.env.OPENTELEMETRY_REDIS_PORT || '63790', 10), | |||
| }; | |||
|
|
|||
| const URL = `redis://${CONFIG.host}:${CONFIG.port}`; | |||
| const REDIS_URL = `redis://${CONFIG.host}:${CONFIG.port}`; | |||
There was a problem hiding this comment.
Reviewer note: I could revert this variable name change. I had changed it originally because at one point I had been using the URL object.
There was a problem hiding this comment.
It's okay to keep the change imo. 🙂
This is an attempt to test ESM usage of ioredis based on Marc's comment at open-telemetry#1731 (comment) This *passes*, but the thing that scares me is enabling the ESM hook for .test.ts tests as well. The IORedisInstrumentation patch method is being called *both* for ESM and CommonJS for ioredis.test.ts -- I don't understand that. Refs: open-telemetry#1731 Refs: open-telemetry#1735
|
We discussed today in the SIG going ahead with this approach. Note that I opened a draft attempt of the other approach at #1752 with some questions/concerns.
@pichlermarc Let me know if you'd prefer me to update this PR to have it avoid using |
|
The build is failing because it needs an update to all otel core deps to 0.45.0 (equivalent to #1725 for the new release). I'm not sure which "schedule" entry in "renovate.json" is relevant. |
| * Limitations: This only supports traces so far, no metrics or logs. | ||
| * There is little error checking here; we are assuming valid OTLP. |
There was a problem hiding this comment.
I'd say this is likely fine. We can expand this later if necessary.
Little error checking also sounds okay to me, since it's for testing.
| @@ -51,13 +51,13 @@ const CONFIG = { | |||
| port: parseInt(process.env.OPENTELEMETRY_REDIS_PORT || '63790', 10), | |||
| }; | |||
|
|
|||
| const URL = `redis://${CONFIG.host}:${CONFIG.port}`; | |||
| const REDIS_URL = `redis://${CONFIG.host}:${CONFIG.port}`; | |||
There was a problem hiding this comment.
It's okay to keep the change imo. 🙂
Running scripts out-of-process for testing allows running tests with different node options and with isolation. The former is useful for ESM testing. This change includes adding an ESM test for ioredis. This depends on #1694 to pass.
Closes: #1731
Which problem is this PR solving?
Short description of the changes
(Note: This is one option for #1731. An alternative is being discussed over there.)
runTestFixture(...)test utility function that takes a script to run out-of-process. It is expected that the script uses OTel tracing usingNodeSDKfrom@opentelemetry/sdk-node. The utility first starts aTestCollectorHTTP server that can collect OTLP (using the HTTP/JSON flavour). Then it execs the script withOTEL_env vars set so thatNodeSDKwill export to that collector. When the script finishes, a couple given callbacks,checkResultandcheckCollector, are called so that the mocha test code can assert expectations about the process result and the collected spans. TheTestCollectorprovides acollector.sortedSpansthat returns the exported spans sorted by start-time.client.set(...)andclient.get(...). An added case in "ioredis.test.ts" runs that and asserts that the script exited successfully and exported the expected spans.@pichlermarc and others interested, I would love some feedback on this.
Checklist
startTimeUnixNanoserialization