Skip to content

Fix flaky maxSubmitRetries test #227

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jul 23, 2018
Merged

Fix flaky maxSubmitRetries test #227

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0518ab7
Fix flaky maxSubmitRetries test
Jul 19, 2018
032eb98
Work around circular sharedb and sharedb-mingo-memory dependency
Jul 19, 2018
e9ff681
Document race condition test fix
Jul 20, 2018
08eb790
Merge branch 'master' into fix-flaky-test
nateps Jul 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,6 @@ node_js:
- "8"
- "6"
- "4"
script: "npm run jshint && npm run test-cover"
script: "ln -s .. node_modules/sharedb; npm run jshint && npm run test-cover"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping another note for the future, this is to work around the circular dependency issue between sharedb and sharedb-mingo-memory:
#226 (comment)

I'm OK with this workaround for now to unblock the tests as the workaround is isolated to Travis, until we figure out a better solution in the linked issue above.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, maybe it's better to get the #226 merged first, as it also updates the Travis config's Node versions to reflect latest + LTE versions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Happy to wait and rebase. Would just be nice to get this through to unblock other PRs.

# Send coverage data to Coveralls
after_script: "cat ./coverage/lcov.info | ./node_modules/coveralls/bin/coveralls.js"
29 changes: 18 additions & 11 deletions test/client/submit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,25 +509,32 @@ describe('client submit', function() {
});

it('submits fail above the backend.maxSubmitRetries threshold', function(done) {
var backend = this.backend;
this.backend.maxSubmitRetries = 0;
var doc = this.backend.connect().get('dogs', 'fido');
var doc2 = this.backend.connect().get('dogs', 'fido');
doc.create({age: 3}, function(err) {
if (err) return done(err);
doc2.fetch(function(err) {
if (err) return done(err);
var count = 0;
var cb = function(err) {
count++;
if (count === 1) {
if (err) return done(err);
} else {
expect(err).ok();
done();
var docCallback;
var doc2Callback;
backend.use('commit', function (request, callback) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I and future contributors (like future you!) would appreciate it if you added some comments here about why this is needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if (request.op.op[0].na === 2) docCallback = callback;
if (request.op.op[0].na === 7) doc2Callback = callback;

if (docCallback && doc2Callback) {
docCallback();
}
};
doc.submitOp({p: ['age'], na: 2}, cb);
doc2.submitOp({p: ['age'], na: 7}, cb);
});
doc.submitOp({p: ['age'], na: 2}, function (error) {
if (error) return done(error);
doc2Callback();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can move this up to just below docCallback(), as by that point both clients will have already fetched the snapshot in submit and gone through apply.

Having the doc2Callback() call up there makes it a bit more obvious that the middleware is deferring the first callback until the second one is ready, and calling both in sequence.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. But isn't the commit hook just before they've been committed to the db? ie there's still a potential race condition if one of them gets committed before the other. That is, we don't know which callback will finish first, and get forced into retry...? This way we guarantee that doc will always be committed before doc2.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(That could be mitigated by putting back the old shared callback with a count, but I find that sort of thing a bit sketchy; it's like admitting that you're not really in control of your own test.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is, we don't know which callback will finish first, and get forced into retry...? This way we guarantee that doc will always be committed before doc2.

(That could be mitigated by putting back the old shared callback with a count, but I find that sort of thing a bit sketchy; it's like admitting that you're not really in control of your own test.)

Got it, you want to sequence the actual commits so you know 1 goes before 2. Makes sense, thanks for explaining!

});
doc2.submitOp({p: ['age'], na: 7}, function (error) {
expect(error).ok();
done();
});
});
});
});
Expand Down