test: sort config value assertions #5899
Conversation
Overall package sizeSelf size: 9.66 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.6.0 | 30.47 MB | 30.47 MB | | @datadog/native-appsec | 8.5.2 | 19.33 MB | 19.34 MB | | @datadog/native-iast-taint-tracking | 4.0.0 | 11.72 MB | 11.73 MB | | @datadog/pprof | 5.8.2 | 9.56 MB | 9.93 MB | | @opentelemetry/core | 1.30.1 | 908.66 kB | 7.16 MB | | protobufjs | 7.5.3 | 2.95 MB | 5.6 MB | | @datadog/wasm-js-rewriter | 4.0.1 | 2.85 MB | 3.58 MB | | @datadog/native-metrics | 3.1.1 | 1.02 MB | 1.43 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | import-in-the-middle | 1.14.0 | 120.58 kB | 841.68 kB | | source-map | 0.7.4 | 226 kB | 226 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | lru-cache | 7.18.3 | 133.92 kB | 133.92 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 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 | 5.3.2 | 53.63 kB | 53.63 kB | | istanbul-lib-coverage | 3.2.2 | 34.37 kB | 34.37 kB | | rfdc | 1.4.1 | 27.15 kB | 27.15 kB | | @isaacs/ttlcache | 1.4.1 | 25.2 kB | 25.2 kB | | dc-polyfill | 0.1.9 | 25.11 kB | 25.11 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | shell-quote | 1.8.2 | 23.54 kB | 23.54 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 | | koalas | 1.0.2 | 6.47 kB | 6.47 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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5899 +/- ##
=======================================
Coverage 80.73% 80.73%
=======================================
Files 462 462
Lines 19914 19914
=======================================
Hits 16078 16078
Misses 3836 3836 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Datadog ReportBranch report: ✅ 0 Failed, 1256 Passed, 0 Skipped, 20m 27.79s Total Time |
| expect(config).to.have.property('flushInterval', 2000) | ||
| expect(config).to.have.property('flushMinSpans', 1000) | ||
| expect(config).to.have.property('queryStringObfuscation').with.length(626) | ||
| expect(config).to.have.nested.property('apmTracingEnabled', true) |
There was a problem hiding this comment.
If we change these, can we please change it to a deepStrictEqual call instead? That way it should be much easier to read.
There was a problem hiding this comment.
I doubt that it will stay sorted over time 😅
They might not stay 100% sorted over time, but it's always easier to come back and tidy things up when it started out as sorted. Today, you just give up and add your new config where ever, but if it was 99% sorted already, it's easier to tidy things up when you're making other changes.
If we change these, can we please change it to a deepStrictEqual call instead? That way it should be much easier to read.
I prefer to keep this PR as quick and simple as possible. I don't want to spend any more time on it than I have to as I'm working on adding a new config and didn't want to keep just adding it where ever there was room 😅
There was a problem hiding this comment.
Could you not use copilot to refactor that while looking at something else? :)
I guess it would be capable of doing that quite well.
There was a problem hiding this comment.
It's going to make it complicated that some of the tests are checking the length of a given value and that the tap output for large assertions is garbage. E.g. it will just give you assertion errors like this:
expected Config{ …(109) } to have deep property 'appsec' of { apiSecurity: { …(2) }, …(14) }, but got { apiSecurity: { …(2) }, …(14) }
There was a problem hiding this comment.
deepStrictEqual doesn't make it much better 🤔
There was a problem hiding this comment.
For example
- expect(config).to.have.deep.property('serviceMapping', {
- a: 'aa',
- b: 'bb'
- })
+ expect(config).to.have.deep.property('serviceMapping', { a: 'aa', b: 'bb' })There was a problem hiding this comment.
About half the changes in this PR was automatic sorting, the other half, I moved lines around manually where the automatic sorting came up short
There was a problem hiding this comment.
Though a bit of a hack, you can better get an idea of the actual differences if you run this command:
diff <(sort packages/dd-trace/test/config.spec.js) <(git show HEAD^:packages/dd-trace/test/config.spec.js | sort)There was a problem hiding this comment.
Actually, this diff is smaller as it ignores trailing commas (which has been altered when items moved to/from the end of lists/arrays):
git show HEAD^:packages/dd-trace/test/config.spec.js | sed 's/,\s*$//' | sort | diff - <(sed 's/,\s*$//' packages/dd-trace/test/config.spec.js | sort)There was a problem hiding this comment.
And here's the output of that command:
644d643
< name: 'sampler.rules'
655d653
< origin: 'env_var'
723d720
< value: process.env.DD_TRACE_SAMPLING_RULES
766c763
< {
---
> { id: '68e75c48-57ca-4a12-adfc-575c4b05fcbe', type: 'k8s_single_step', time: '1703188212' })
958a956
> { name: 'sampler.rules', value: process.env.DD_TRACE_SAMPLING_RULES, origin: 'env_var' }
1067d1064
< }
1122,1123d1118
< a: 'aa'
< a: 'aa'
1144,1145d1138
< b: 'bb'
< b: 'bb'
1151d1143
< c: 'cc'
1217d1208
< d: 'dd'
1378d1368
< id: '68e75c48-57ca-4a12-adfc-575c4b05fcbe'
1564d1553
< time: '1703188212'
1575d1563
< type: 'k8s_single_step'
1679,1682d1666
< })
< })
< })
< })
1847d1830
< expect(config.tags).to.have.property('foo', 'bar')
1888,1889c1871,1872
< expect(config).to.have.deep.property('installSignature', {
< expect(config).to.have.deep.property('peerServiceMapping', {
---
> expect(config).to.have.deep.property('installSignature'
> expect(config).to.have.deep.property('peerServiceMapping', { c: 'cc', d: 'dd' })
1892,1893c1875,1876
< expect(config).to.have.deep.property('serviceMapping', {
< expect(config).to.have.deep.property('serviceMapping', {
---
> expect(config).to.have.deep.property('serviceMapping', { a: 'aa', b: 'bb' })
> expect(config).to.have.deep.property('serviceMapping', { a: 'aa', b: 'bb' })
|
Just force pushed to try and trigger CI again as there's a bug in CI that might be fixable by re-running from a fresh SHA. No code-changes was added in the force-push |
ghost
mentioned this pull request
What does this PR do?
Sort the assertions in the major tests in
config.spec.js.Motivation
Less bike-shedding in the future when modifying these tests.
Plugin Checklist
Additional Notes
One duplicate assertion found during refactoring which has been removed. Besides that, the only changes are in the order