Drop Testing for Segfault Couchbase Versions #2252
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2252 +/- ##
==========================================
+ Coverage 92.94% 93.49% +0.55%
==========================================
Files 220 218 -2
Lines 8515 8966 +451
==========================================
+ Hits 7914 8383 +469
+ Misses 601 583 -18
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Overall package sizeSelf size: 2.58 MB Dependency sizes
🤖 This report was automatically generated by heaviest-objects-in-the-universe |
| @@ -1,7 +1,7 @@ | |||
| version: "2" | |||
| services: | |||
| couchbase: | |||
| image: sabrenner/couchbase-server-sandbox:latest | |||
| image: sabrenner/couchbase-server-sandbox:sandbox | |||
There was a problem hiding this comment.
Just a different version of the image I pushed up. It uses a different parent image, but I wanted to keep the other one in the case that it could still be useful, so I just named this one something different.
| const startCh = channel('apm:couchbase:query:start') | ||
| const finishCh = channel('apm:couchbase:query:finish') | ||
| const errorCh = channel('apm:couchbase:query:error') | ||
| if (!semver.satisfies(process.version, '>=14.0.0')) { |
There was a problem hiding this comment.
Can this be done in the test instead?
There was a problem hiding this comment.
I added this here because, when running tests in the couchbase job (v3 and higher), using Node 14, it would still try and build v2 using that Node version as well, which would cause a crash (and, trying to run v3 and higher tests with Node 12, which is what works with couchbase v2, would also cause it to crash). So, I put this here to not even build couchbase v2 when using Node 14 and higher. If there is a way I can add this to the tests instead, I will!
There was a problem hiding this comment.
Seems to me like it'd be valuable to just add a nodeVersions option to the addHook(options, ...) input so checks like this could be done anywhere.
There was a problem hiding this comment.
This is something I'd be happy to add in a separate PR. It would definitely help in this case too in cleaning up the tests.
There was a problem hiding this comment.
I added this here because, when running tests in the couchbase job (v3 and higher), using Node 14, it would still try and build v2 using that Node version as well
This is basically my point, if this code is only for tests it should be in tests. If someone gets couchbase to build on Node 14 for example, there is no reason why our instrumentation shouldn't work. An option could be added to externals.json for example.
| addHook({ | ||
| name: 'couchbase', | ||
| file: 'dist/collection.js', | ||
| versions: ['3.2.0 - 3.2.1', '3.2.2 - 3.2.4', '4.0.0', '^4.1.0'] }, collection => { |
There was a problem hiding this comment.
Additional ranges can be added to externals.json instead.
There was a problem hiding this comment.
Especially since this range won't support new versions automatically.
| cluster.authenticate('Administrator', 'password') | ||
| cluster.enableCbas('localhost:8095') | ||
| bucket = cluster.openBucket('datadog-test', (err) => done(err)) | ||
| if (VERSION === 'v2') { |
There was a problem hiding this comment.
Why is a completely different test suite needed for v2 and v3? Could most of the code be reused and just have a different setup function for example?
There was a problem hiding this comment.
Hmm maybe, but there are just enough small differences in some tests, and some checks, that I decided to have them separate. I might just be able to remove the envvar check, but that would rely on keeping the change two comments above (restricting node versions, which for now is in the integration, but again, if there's a way I can move that to the tests, I definitely will!).
| } else { | ||
| const ignoreVersions = version => | ||
| semver.intersects(version, '3.2.0 - 3.2.1') || // related to JSCBC-916 | ||
| semver.intersects(version, '^4.1.0') // multiple spdlog error |
There was a problem hiding this comment.
Could we ignore 4.0.0 instead of ignoring the latest version? Otherwise we won't know if something breaks with a new release of the library.
There was a problem hiding this comment.
Yeah I'll give that a shot, but I would probably have to take more of a look into why there is some multiple loggers exception. This happens when testing any multiple versions of 4.x together (so, if I tested ^4.1.0, it would test 4.1.0 and whatever latest 4.x version there is, triggering the exception. I used 4.0.0 as a single version to avoid this, but also had the same thought you brought up before pushing this change up but thought I'd see what you'd say, so I'll look into it!).
…d-trace-js into sabrenner/couchbasesegfaultfix
There was a problem hiding this comment.
As far as I can tell this was addressed by #3265 - the test remains very occasionally flaky, but that seems to be down to the server image.
@sabrenner can we close this?
What does this PR do?
Removes testing on couchbase versions 3.2.0 - 3.2.1 and 4.1.x, following intermittent segfaults in our CI.
Motivation
It would be nice to see clean CI runs, and the issues present here are most likely related to the way this plugin is being tested now on multiple versions, and not that the plugin itself is broken on our end (as it still passes locally and in most of CI cases). This is only a temporary change, pending more investigation into where the segfaults are coming from.
All of these commits/attempts resulted in the following conclusions/reasons for the segfaults (Note: these versions are still supported in instrumentation, they are just skipped in our tests):
3.2.0 - 3.2.1: Intermittent segfaults present, fixed in 3.2.2 (https://forums.couchbase.com/t/segfault-in-connection-desctructor-using-3-2-1/31632) (https://issues.couchbase.com/browse/JSCBC-916)
** 4.1.x**: This might require a bit more exploration on our end, but our testing framework + couchbase doesn't play well when testing successive versions post-4.0, as a multiple logger exception is thrown from the underlying native add-on. This can maybe be resolved in our testing framework, but for now is just ignored.