-
Notifications
You must be signed in to change notification settings - Fork 106
Initial work to test mongodb-core on evergreen #287
Conversation
This just makes our evergreen configuration look a little more like other drivers at very little cost to us. NODE-884
this will pass the option down to `mongodb-download-url` and enable detection of Linux Distributions
This ditches my original custom configuration in favor of one based on the skeleton provided in drivers-evergreen-tools. NODE-884
These tests should either be converted to using the mock server, or we need to update the test configuration to detect whether we are running on evergreen or not, and use the orchestration's rest api to do what we're asking here. NODE-884
This is a low-impact change that allows us to potentially make far more accurate assessments of when certain events have completed in the pool. This also allows us to reduce the chances of flakey pool tests
@@ -8,7 +8,8 @@ var expect = require('chai').expect, | |||
Bson = require('bson'); | |||
|
|||
describe('Basic single server auth tests', function() { |
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.
Is every test skip
ed in this describe
block? We could just do describe.skip
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.
yeah I'm not sure what happened here, I think these were skipped from a while ago but sure I can describe.skip
it.
@@ -2,6 +2,11 @@ | |||
const ConfigurationBase = require('mongodb-test-runner').ConfigurationBase; | |||
const f = require('util').format; | |||
|
|||
const chai = require('chai'); |
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.
Is there a reason we are adding these things in?
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 was actually glad to have found this, we often have cases where we expect(err).to.not.exist
, however if it does it shows something like Expected { Object(namespace, message) } to be null
which is not helpful at all. The truncation configuration option here expands to show the full message. The other options here are just the defaults, but I'm otherwise codifying that we are indeed using the defaults.
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.
Yeah, this sounds useful. Like I said above though, lets make sure it is a separate commit when we squash.
@@ -210,6 +210,7 @@ function stateTransition(self, newState) { | |||
// Get current state | |||
var legalStates = legalTransitions[self.state]; | |||
if (legalStates && legalStates.indexOf(newState) !== -1) { | |||
self.emit('stateChanged', self.state, newState); |
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.
Is this a bug that needs to be fixed?
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 was actually using this to determine when the pool was destroyed, before I opted to use connection spies. It's a harmless addition, but adds flexibility to the design (it's a no-op if nobody is listening) so I'd opt for keeping it in.
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.
Lets make sure that when we squash, this is a separate commit so we can track it.
.evergreen/run-tests.sh
Outdated
download_and_extract "$MONGODB_DOWNLOAD_URL" "$EXTRACT" | ||
|
||
# run tests | ||
OS=$(uname -s | tr '[:upper:]' '[:lower:]') |
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 feel that things like this could be made into bash functions to make it clearer what is happening.
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.
this is a straight rip of the existing mongo-orchestration scripts, soon to be replaced by work Sam Kleinman is doing with the curator
method (read: this is going away soon).
.evergreen/run-tests.sh
Outdated
OS=$(uname -s | tr '[:upper:]' '[:lower:]') | ||
[ -z "$MARCH" ] && MARCH=$(uname -m | tr '[:upper:]' '[:lower:]') | ||
|
||
if [ "$AUTH" != "noauth" ]; then |
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.
Can we take strings like this and stick them in variables? I'm worried about things like noauth
or nossl
being changed in the future.
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 think it's a bit premature, there's only a single reference to it. I'll remove the whole section of code actually, since we aren't presently running auth or ssl tests and we aren't likely to use this pattern anyway.
.evg.yml
Outdated
- id: node-version | ||
display_name: "Node.js" | ||
values: | ||
- id: "Argon" |
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.
Are we not making one of these for Carbon
?
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.
Yes, Carbon is in the .evergreen/config.yml
.evg.yml
Outdated
variables: | ||
NODE_LTS_NAME: "boron" | ||
|
||
buildvariants: |
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.
Does evergreen not support >2D matrices? It would be nice to be able to add an axis for Mongo Version
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.
unfortunately not :'(
@@ -0,0 +1,629 @@ | |||
# When a task that used to pass starts to fail |
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.
what is the difference between this file and .evg.yml
?
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.
ack .evg.yml
was accidentally commited. Removing now, and disregarding review from that file.
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.
ok, then transplant all my comments from .evg.yml
to evergreen/config.yml
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.
honestly I already squashed a whole bunch of commits here, I'm inclined to leave it as is
@daprahamian okay comments should be addressed |
This includes a number of changes, but generally provides the foundation for testing our two drivers in evergreen, closing: NODE-871, NODE-873, NODE-879, NODE-880, NODE-881, NODE-883, NODE-884, NODE-1191. It also includes a few modifications to the actual tests to reduce flakiness of tests.
You can see it all builds in evergreen here: https://evergreen.mongodb.com/version/5ab7939ce3c331475bc1144a. The red boxes in the result are related to node-snappy not being built to support big endian on those ZAP systems, I have a fix for that here