Skip to content

Conversation

mormubis
Copy link
Contributor

@mormubis mormubis commented Oct 6, 2025

Motivation

The initial implementation of Worker E2E tests was somewhat tied to our specific use case and will not scale if we need to test different scenarios within the Worker environment.

Changes

It introduces a version of withWorker that automatically sets up the logs inside the worker. We can also pass an implementation, similar to what we do with withLogs and withBody.

Test instructions

All e2e still passed.

Checklist

  • Tested locally
  • Tested on staging
  • Added unit tests for this change.
  • Added e2e/integration tests for this change.

Copy link

cit-pr-commenter bot commented Oct 6, 2025

Bundles Sizes Evolution

📦 Bundle Name Base Size Local Size 𝚫 𝚫% Status
Rum 162.08 KiB 162.08 KiB 0 B 0.00%
Rum Recorder 19.78 KiB 19.78 KiB 0 B 0.00%
Rum Profiler 4.89 KiB 4.89 KiB 0 B 0.00%
Logs 55.77 KiB 55.77 KiB 0 B 0.00%
Flagging 944 B 944 B 0 B 0.00%
Rum Slim 119.06 KiB 119.06 KiB 0 B 0.00%
Worker 23.60 KiB 23.60 KiB 0 B 0.00%
🚀 CPU Performance
Action Name Base CPU Time (ms) Local CPU Time (ms) 𝚫%
RUM - add global context 0.0045 0.0074 +64.44%
RUM - add action 0.014 0.0147 +5.00%
RUM - add error 0.0122 0.0126 +3.28%
RUM - add timing 0.003 0.0031 +3.33%
RUM - start view 0.0037 0.004 +8.11%
RUM - start/stop session replay recording 0.001 0.0008 -20.00%
Logs - log message 0.0176 0.0133 -24.43%
🧠 Memory Performance
Action Name Base Memory Consumption Local Memory Consumption 𝚫
RUM - add global context 25.46 KiB 25.93 KiB +477 B
RUM - add action 45.97 KiB 45.12 KiB -872 B
RUM - add timing 24.34 KiB 24.92 KiB +596 B
RUM - add error 51.44 KiB 49.84 KiB -1.60 KiB
RUM - start/stop session replay recording 24.89 KiB 24.30 KiB -608 B
RUM - start view 422.83 KiB 426.77 KiB +3.94 KiB
Logs - log message 43.14 KiB 42.29 KiB -870 B

🔗 RealWorld

Copy link

datadog-official bot commented Oct 6, 2025

✅ Tests

🎉 All green!

❄️ No new flaky tests detected
🧪 All tests passed

🎯 Code Coverage
Patch Coverage: 100.00%
Total Coverage: 92.67% (+0.00%)

View detailed report

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 9afa674 | Docs | Was this helpful? Give us feedback!

@mormubis mormubis marked this pull request as ready for review October 6, 2025 12:56
@mormubis mormubis requested a review from a team as a code owner October 6, 2025 12:56
@mormubis mormubis changed the title refactor: change e2e implementation for workers 👷 change e2e implementation for workers Oct 6, 2025
createTest('service worker with worker logs - esm')
.withWorker()
.withLogs()
.withWorker(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to move this to the createTest? So that we do something like:
.withWorker({ isModule: true, handleMessageWith: 'logger' })
Similar to what we do in:

.withExtension(createExtension(path.join(BASE_PATH, name)).withRum().withLogs())

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the idea is to avoid that scenario and be more generic, or that's what I understood from @bcaudan feedback.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could make the API more convenient by:

  • providing a function to withWorker, to work directly with code instead of a block of text
  • have the worker implementation as complete as possible:
    • let it import the script rather than handle that in workerSetup, it would be easier to follow
    • provide it the consolidated configuration

to be able to do something like:

.withWorker((config) => {
  import '/datadog-logs.js'
      
  // Initialize DD_LOGS in service worker
  DD_LOGS.init(config)

  // Handle messages from main thread
  self.addEventListener('message', (event) => {
    const message = event.data
    DD_LOGS.logger.log(message)
  })
})

Copy link
Contributor Author

@mormubis mormubis Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bcaudan I had this as an earlier version, but then I found that the current implementation is closer to what we do with Logs/RUM (withLogs and withRUM).

@mormubis mormubis force-pushed the adlrb/logs-improve-testing branch from 0feb678 to a5b8ded Compare October 10, 2025 09:18
Comment on lines +22 to +27
// Handle messages from main thread
self.addEventListener('message', (event) => {
const message = event.data;
DD_LOGS.logger.log(message);
});
Copy link
Collaborator

@bcaudan bcaudan Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ question: ‏Do we need to use post message for every test? Could we instead have the log needed for the test be triggered when the worker is started from the worker implementation?
It would reduce the complexity of those tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bcaudan I thought about it, and it's a bit tricky, because it means the test it auto-executes it. Are you ok with that?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my understanding the test would instantiate the worker and this instantiation could generate the log use case that we want to test, avoiding the extra message exchanges with the main thread.
Was there any issue that this message exchange was preventing or any other issue that you see with the "auto-execution"?
if not, I'd say it is worth a try.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we do it with other tests as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants