search

DataDog / dd-trace-js

JavaScript APM Tracer
https://docs.datadoghq.com/tracing/
Other
647 stars 306 forks source link

Unable to trace tests with "custom" JSDom environment #1698

Closed clementprevot closed 1 year ago

clementprevot commented 2 years ago

Describe the bug We activated the Datadog tracing agent for our tests a couple of month ago, when we were under Jest 26. The tracing is working perfectly fine, even though we are using a "custom" environment to run our tests. By custom, I mean that we are creating an environment extending the default JSDom one (see below in "Environment")

So as I said, this was perfectly working.

Then we recently tried to migrate to Jest 27. After managing to migrate our suites and everything, we wanted to re-enable tracing with the exact same environment and it's now not working anymore.

When running my tests, I got this kind of error:

● Test suite failed to run

 TypeError: Cannot set properties of undefined (setting '<The title of my test>')

 at CustomEnvironment.handleTestEventWithTrace [as handleTestEvent] (node_modules/dd-trace/packages/datadog-plugin-jest/src/jest-environment.js:189:52)

If a remove the ddtrace.init call, my tests runs perfectly fine.

Could you help me debug this and make the agent work fine with this extended JSDom env 🙏🏻

Environment

const JSDOMEnvironment = require('jest-environment-jsdom')

// Since the JSDOM environment is the one provided by Jest by default, we
// stick to it, and create our custom implementation based on this one.
class CustomEnvironment extends JSDOMEnvironment {
  constructor(config, context) {
    super(config, context)

    // JSDOM does not define this `scrollIntoView` method, so we have to
    // manually add it.
    this.dom.window.Element.prototype.scrollIntoView = () => {}

    // JSDOM does not have any ResizeObserver implementation.
    this.dom.window.ResizeObserver = class ResizeObserver {
      disconnect() {}

      unobserve() {}

      observe() {}
    }
  }
}

require('dd-trace').init({
  service: 'xxxx',
  flushInterval: 300000,
})

module.exports = CustomEnvironment
rochdev commented 2 years ago

@juan-fernandez Looks like there might be a bug in the jest plugin?

@clementprevot While this is being fixed, you could disable the jest plugin so that you can continue to get traces while avoiding the errors. You would be missing any jest span but at least you would get everything else. You can disable the plugin with tracer.use('jest', false).

clementprevot commented 2 years ago

@clementprevot While this is being fixed, you could disable the jest plugin so that you can continue to get traces while avoiding the errors. You would be missing any jest span but at least you would get everything else. You can disable the plugin with tracer.use('jest', false).

Despite the fact that I disabled the plugin using your above code, I still have the error, am I doing something wrong?

/* eslint-disable max-classes-per-file */

const DatadogTracer = require('dd-trace')
const JSDOMEnvironment = require('jest-environment-jsdom')

// Since the JSDOM environment is the one provided by Jest by default, we
// stick to it, and create our custom implementation based on this one.
class CustomEnvironment extends JSDOMEnvironment {
  constructor(config, context) {
    super(config, context)

    // JSDOM does not define this `scrollIntoView` method, so we have to
    // manually add it.
    this.dom.window.Element.prototype.scrollIntoView = () => {}

    // JSDOM does not have any ResizeObserver implementation.
    /* eslint-disable class-methods-use-this */
    this.dom.window.ResizeObserver = class ResizeObserver {
      disconnect() {}

      unobserve() {}

      observe() {}
    }
    /* eslint-enable class-methods-use-this */
  }
}

// This will trace and logs tests in Datadog.
const tracer = DatadogTracer.init({
  enabled: process.env.DD_ENV === 'ci',
  service: 'pastrami',
  // To guarantee test span delivery.
  flushInterval: 300000,
})

// It seems that there is an issue with the `jest` plugin of the `dd-trace`
// library.
// We've been recommended to disabled this plugin for the moment to still enjoy
// test tracing without having Jest spans.
// See: https://github.com/DataDog/dd-trace-js/issues/1698#issuecomment-985790543
tracer.use('jest', false)

module.exports = CustomEnvironment
rochdev commented 2 years ago

@clementprevot Both tracer.init() and tracer.use() should be called before jest-environment-jsdom is imported, otherwise it's too late to disable the plugin.

clementprevot commented 2 years ago

@clementprevot Both tracer.init() and tracer.use() should be called before jest-environment-jsdom is imported, otherwise it's too late to disable the plugin.

Ah! Good to know! Let me try that. Thanks a lot for your help!

clementprevot commented 2 years ago

Well, you solved it. Even without disabling the jest plugin, it now works perfectly fine if I init the tracer before importing jest-environment-jsdom.

Thanks a lot for you help again!

rochdev commented 2 years ago

@juan-fernandez This would still be worth fixing as we shouldn't cause an error even when the tracer is not initialized at the right time.

clementprevot commented 2 years ago

@rochdev I have a quick update on this: in my previous comment I said that it was working fine, but it occurs in the end that it's not exactly true. Most of our tests run fine, but some of them fails when running with the tracer (with jest plugin) enabled while they run fine when the tracer is disabled.

I also tried to run with the tracer enabled but the jest plugin disabled, and in this case the test are all running fine.

So I re-open this issue

juan-fernandez commented 2 years ago

hi @clementprevot!

Thanks for reporting this issue.

The first thing to tackle is the initialization order: require('dd-trace').init() should always be before requiring your custom environment. In this case: 

require('dd-trace').init({
  enabled: process.env.DD_ENV === 'ci',
  service: 'pastrami',
  // To guarantee test span delivery.
  flushInterval: 300000,
})
const JSDOMEnvironment = require('jest-environment-jsdom')

class CustomEnvironment extends JSDOMEnvironment {
  // ...
}
module.exports = CustomEnvironment

This said, the issue seems to be a version mismatch. Could you provide me with the following information:

In any case, we should never break tests, even when jest versions are mismatching, so we'll handle this as a bug on our side.

clementprevot commented 2 years ago

Hi @juan-fernandez

Thank you for your answer.

I indeed moved the initialization of the tracer above the requiring of th the JSDOM env. It fixed most of the tests, but some of them are still failing (but not with the same error as the one mentionned in the issue, some "random" errors, different, depending on the test). And disabling the jest plugin fixes all the tests

Here are the information you asked:

There are indeed some version mismatch. I will try to sort this out and keep you informed if it works better when everything is aligned.

clementprevot commented 2 years ago

Updating everything to the same version number didn't changed the issue with some of the tests. I still have to disable the jest plugin

juan-fernandez commented 2 years ago

Updating everything to the same version number didn't changed the issue with some of the tests. I still have to disable the jest plugin

Thanks for trying that.

A couple of questions:

clementprevot commented 2 years ago

Nope the error is not the same as the one mentionned in the first message of this issue. And it's not the same error for all tests. At first, those error looks like "legit" error of the unit tests (some cannot access something of undefined, some Testing library "cannot find element", ...) but they are definitely not due to the test themselves as as soon as I disabled the tracer (or the jest plugin) all tests pass.

Let me try to find something in common between those tests and some examples of errors. I'll keep you posted

juan-fernandez commented 2 years ago

thanks! It's definitely a bug on our side. By understanding when it's happening exactly the fix will have a bigger chance of solving this and future issues 😄

juan-fernandez commented 2 years ago

hey @clementprevot! Is there any way for you to try to use the newest release https://github.com/DataDog/dd-trace-js/releases/tag/v2.4.2? We fixed the way the environment is tore down in https://github.com/DataDog/dd-trace-js/pull/1965 and this seems to have fixed issues for a couple of users.

In any case, we're in the process of rewriting the jest instrumentation, to simplify it and make this kind of problems less likely.

clementprevot commented 2 years ago

Hi @juan-fernandez

I can try this release. I'll let you know how it went!

Thanks for letting me know!

juan-fernandez commented 2 years ago

hey @clementprevot. Were you able to test this? Additionally, from >=2.6.0 the jest instrumentation was reworked and it should be more stable now.

clementprevot commented 2 years ago

@juan-fernandez Sorry about the delay, I had some health issues recently and haven't been often at work. Sadly, the 2.4.2 release didn't solved my problem and I still had soem tests failing only when the instrumentation was in place.

I'll will try the 2.6.0 version though.

juan-fernandez commented 2 years ago

@juan-fernandez Sorry about the delay, I had some health issues recently and haven't been often at work. Sadly, the 2.4.2 release didn't solved my problem and I still had soem tests failing only when the instrumentation was in place.

I'll will try the 2.6.0 version though.

oh, that's OK, take your time 😄 and I hope you get better! And thanks for testing this!

clementprevot commented 2 years ago

OK, I tested with v2.7.0 and I still have some weird behaviour.

Let's take this example:

const activeMarkets = [
  markets[COUNTRIES.DE],
  markets[COUNTRIES.AT],
  markets[COUNTRIES.ES],
]

it.each(activeMarkets)(
    'shows the active markets if the merchant is online',
    ({ countryCode }) => {
      // Pass the active markets to the component. This function is omitted here.
      setup(activeMarkets)

      expect(screen.getByText(countryCode)).toBeInTheDocument()
    },
  )

Without instrumentation, this test works perfectly fine. With instrumentation, it fails.

[EDIT] The failure is pretty weird. The first iteration of the it will only have the last two element in the page, the second iteration only the last and the third iteration, not element at all [/EDIT]

However, still with instrumentation, if I replace it.each(activeMarkets) by it.each([...activeMarkets]) now my test works again.

So I may have a workaround but the issue is that when running test in local computer, instrumentation is not enabled. It's only enabled on CI. This means that devs won't have any error in their tests in local, but tests will crash in the CI, without any known reason.

Do you have a clue of why I have such a behaviour?

juan-fernandez commented 2 years ago

OK, I tested with v2.7.0 and I still have some weird behaviour.

Let's take this example:

const activeMarkets = [
  markets[COUNTRIES.DE],
  markets[COUNTRIES.AT],
  markets[COUNTRIES.ES],
]

it.each(activeMarkets)(
    'shows the active markets if the merchant is online',
    ({ countryCode }) => {
      // Pass the active markets to the component. This function is omitted here.
      setup(activeMarkets)

      expect(screen.getByText(countryCode)).toBeInTheDocument()
    },
  )

Without instrumentation, this test works perfectly fine. With instrumentation, it fails.

[EDIT] The failure is pretty weird. The first iteration of the it will only have the last two element in the page, the second iteration only the last and the third iteration, not element at all [/EDIT]

However, still with instrumentation, if I replace it.each(activeMarkets) by it.each([...activeMarkets]) now my test works again.

So I may have a workaround but the issue is that when running test in local computer, instrumentation is not enabled. It's only enabled on CI. This means that devs won't have any error in their tests in local, but tests will crash in the CI, without any known reason.

Do you have a clue of why I have such a behaviour?

thanks for the information. There seems to be a sneaky bug in our code. Let me run some tests and I'll get back to you.

juan-fernandez commented 1 year ago

const activeMarkets = [ markets[COUNTRIES.DE], markets[COUNTRIES.AT], markets[COUNTRIES.ES], ]

it.each(activeMarkets)( 'shows the active markets if the merchant is online', ({ countryCode }) => { // Pass the active markets to the component. This function is omitted here. setup(activeMarkets)

  expect(screen.getByText(countryCode)).toBeInTheDocument()
},

)

hey! Sorry for the late reply: I just attempted to reproduce this and I couldn't. Your example seems to work just fine. I'll close this issue, but feel free to reopen if it's still happening to you. If it is, I'd really appreciate a small reproducible case 😄