- 
                Notifications
    You must be signed in to change notification settings 
- Fork 351
split sandbox between linked and isolated modes #6730
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
| Overall package sizeSelf size: 12.89 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.7.0 | 35.02 MB | 35.02 MB | | @datadog/native-appsec | 10.3.0 | 20.73 MB | 20.74 MB | | @datadog/native-iast-taint-tracking | 4.0.0 | 11.72 MB | 11.73 MB | | @datadog/pprof | 5.11.1 | 9.96 MB | 10.34 MB | | @opentelemetry/core | 1.30.1 | 908.66 kB | 7.16 MB | | protobufjs | 7.5.4 | 2.95 MB | 5.73 MB | | @datadog/wasm-js-rewriter | 4.0.1 | 2.85 MB | 3.58 MB | | @opentelemetry/resources | 1.9.1 | 306.54 kB | 1.74 MB | | @datadog/native-metrics | 3.1.1 | 1.02 MB | 1.43 MB | | @opentelemetry/api-logs | 0.206.0 | 201.39 kB | 1.42 MB | | @opentelemetry/api | 1.9.0 | 1.22 MB | 1.22 MB | | jsonpath-plus | 10.3.0 | 617.18 kB | 1.08 MB | | import-in-the-middle | 1.15.0 | 127.66 kB | 856.24 kB | | lru-cache | 10.4.3 | 804.3 kB | 804.3 kB | | @datadog/openfeature-node-server | 0.1.0-preview.12 | 95.11 kB | 401.68 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | source-map | 0.7.6 | 185.63 kB | 185.63 kB | | pprof-format | 2.2.1 | 163.06 kB | 163.06 kB | | @datadog/sketches-js | 2.1.1 | 109.9 kB | 109.9 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | ignore | 7.0.5 | 63.38 kB | 63.38 kB | | istanbul-lib-coverage | 3.2.2 | 34.37 kB | 34.37 kB | | rfdc | 1.4.1 | 27.15 kB | 27.15 kB | | dc-polyfill | 0.1.10 | 26.73 kB | 26.73 kB | | @isaacs/ttlcache | 1.4.1 | 25.2 kB | 25.2 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | shell-quote | 1.8.3 | 23.74 kB | 23.74 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | semifies | 1.0.0 | 15.84 kB | 15.84 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | ttl-set | 1.0.0 | 4.61 kB | 9.69 kB | | mutexify | 1.4.0 | 5.71 kB | 8.74 kB | | path-to-regexp | 0.1.12 | 6.6 kB | 6.6 kB | | module-details-from-path | 1.0.4 | 3.96 kB | 3.96 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe | 
| Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@            Coverage Diff             @@
##           master    #6730      +/-   ##
==========================================
+ Coverage   84.13%   84.19%   +0.05%     
==========================================
  Files         505      509       +4     
  Lines       21038    21123      +85     
==========================================
+ Hits        17701    17785      +84     
- Misses       3337     3338       +1     ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
 | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM. I didn't understand the addition of the modules in the versions package.json though.
| "@prisma/client": "6.17.1", | ||
| "@redis/client": "5.8.3", | ||
| "@smithy/smithy-client": "4.9.0", | ||
| "@types/node": "18.19.130", | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am surprised about this addition. I believe that should not have an impact, since it is already listed in our main package.json.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's needed by the test app. It was available before because it got hoisted from dd-trace to the test app node_modules which is no longer the case when linking.
| "couchbase": "4.6.0", | ||
| "cypress": "15.4.0", | ||
| "cypress-fail-fast": "7.1.1", | ||
| "dc-polyfill": "0.1.10", | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about this change. Why do we need to add that to the versions package.json?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's needed by the test app. It was available before because it got hoisted from dd-trace to the test app node_modules which is no longer the case when linking.
| */ | ||
| async function createSandbox (dependencies = [], isGitRepo = false, | ||
| integrationTestsPaths = ['./integration-tests/*'], followUpCommand) { | ||
| integrationTestsPaths = ['./integration-tests/*'], followUpCommand, isolated = true) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not really need the default value, since it is explicitely passed through all the time.
I would probably change the arguments order to make the default values arguments at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also used by useSandbox, but I guess it could also pass it explicitly.
| execHelper(addCommand, addOptions) | ||
| if (isolated) { | ||
| execHelper(`npm pack --silent --pack-destination ${folder}`, { env: restOfEnv }) | ||
| execHelper(`yarn add ${deps.concat(`file:${out}`).join(' ')} ${addFlags.join(' ')}`, addOptions) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| execHelper(`yarn add ${deps.concat(`file:${out}`).join(' ')} ${addFlags.join(' ')}`, addOptions) | |
| execHelper(`yarn add ${deps.join(' ')} `file:${out}` ${addFlags.join(' ')}`, addOptions) | 
| @BridgeAR Some of your comments around dependencies made me realize that by moving to linking, we lose our only safety net that guarantees that we're not using a dev dependency by accident in an integration. Since they are lazy loaded, the existing integration doesn't otherwise cover them. Because of this, I think we should keep all integration tests isolated. The problem we were trying to solve also no longer exists after the switch to Bun, not only because it's really fast to begin with, but it's also able to reuse the dependencies that were pulled from the install on the repo itself, so it doesn't need to pull the packages again from internet. With that said and given there are basically no performance benefits and we lose some important aspects of these tests, I would prefer to close this PR and keep the isolated sandbox always. WDYT? cc @bengl | 
What does this PR do?
Split sandbox between linked and isolated modes.
Motivation
The sandbox was originally created for integration tests with applications mimicking a real app with a real install of dd-trace. But for a lot of our tests, for example ESM tests, we don't really need that level of isolation, and linking to dd-trace is good enough.
Additional Notes
The main change is in
integration-tests/helpers/index.js, everything else is just changing all usage ofcreateSandboxto eitherisolatedSandboxorlinkedSandbox.Supersedes #6640