dd-trace-py icon indicating copy to clipboard operation
dd-trace-py copied to clipboard

chore(tests): add testing to ensure patching does not import module

Open ZStriker19 opened this issue 1 year ago • 2 comments

Following up on 5455 .

We should not be importing the module when we patch. This PR adds an assert to the patch testing to make sure that we do not. It should fail on Django, and likely other integrations currently. From there we'll need to implement fixes.

Checklist

  • [ ] Change(s) are motivated and described in the PR description.
  • [ ] Testing strategy is described if automated tests are not included in the PR.
  • [ ] Risk is outlined (performance impact, potential for breakage, maintainability, etc).
  • [ ] Change is maintainable (easy to change, telemetry, documentation).
  • [ ] Library release note guidelines are followed.
  • [ ] Documentation is included (in-code, generated user docs, public corp docs).
  • [ ] PR description includes explicit acknowledgement/acceptance of the performance implications of this PR as reported in the benchmarks PR comment.

Reviewer Checklist

  • [ ] Title is accurate.
  • [ ] No unnecessary changes are introduced.
  • [ ] Description motivates each change.
  • [ ] Avoids breaking API changes unless absolutely necessary.
  • [ ] Testing strategy adequately addresses listed risk(s).
  • [ ] Change is maintainable (easy to change, telemetry, documentation).
  • [ ] Release note makes sense to a user of the library.
  • [ ] Reviewer has explicitly acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment.

ZStriker19 avatar May 02 '23 15:05 ZStriker19

Benchmarks

Comparing candidate commit 791c98627 in PR branch add_assert_module_not_imported_on_patch_testing with baseline commit 7d875a495 in branch 1.x.

Found 1 performance improvements and 0 performance regressions! Performance is the same for 93 cases.

scenario:flasksimple-tracer-and-profiler-and-appsec

  • 🟩 execution_time [-0.571ms; -0.473ms] or [-8.756%; -7.249%]

pr-commenter[bot] avatar May 02 '23 16:05 pr-commenter[bot]

Should we convert this PR to a draft? There are 54 failing checks. Some additional work is probably required.

mabdinur avatar May 18 '23 17:05 mabdinur