Skip to content
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

Fix admin products publication slowness #4260

Merged
merged 11 commits into from
Jun 4, 2018
Merged

Fix admin products publication slowness #4260

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e40f792
fix: no hang on publishing many "Products" for admins
aldeed May 21, 2018
02cc44f
fix: incorporate fix and refactor from #4259 @pmn4
aldeed May 30, 2018
fc86323
Merge branch 'release-2.0.0' into fix-4040-aldeed-fast-pub-admin-prod…
aldeed May 30, 2018
ddfb1c9
tests: fix merge issue
aldeed May 30, 2018
28b95fe
tests: add timeouts to stop random test failures
aldeed Jun 1, 2018
bbd5252
tests: fix the default shop fixture
aldeed Jun 1, 2018
b869e65
fix: editMode default is now false for Products
aldeed Jun 1, 2018
618cb76
tests: fix product publication tests
aldeed Jun 1, 2018
064b7f8
tests: remove console log
aldeed Jun 1, 2018
943ab49
Merge branch 'release-1.13.0' into fix-4040-aldeed-fast-pub-admin-pro…
aldeed Jun 4, 2018
b8af239
chore: fix eslint issues in test file
aldeed Jun 4, 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
6 changes: 4 additions & 2 deletions imports/collections/schemas/products.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,7 +432,8 @@ export const Product = new SimpleSchema({
},
"ancestors": {
type: Array,
defaultValue: []
defaultValue: [],
index: 1
},
"ancestors.$": {
type: String
Expand Down Expand Up @@ -572,7 +573,8 @@ export const Product = new SimpleSchema({
},
"createdAt": {
type: Date,
autoValue: createdAtAutoValue
autoValue: createdAtAutoValue,
index: 1
},
"updatedAt": {
type: Date,
Expand Down
43 changes: 37 additions & 6 deletions imports/plugins/core/job-collection/lib/jobCollection.app-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ const validJobDoc = (d) => Match.test(d, defaultColl.jobDocPattern);

describe("JobCollection default constructor", function () {
it("should be an instance of JobCollection", function () {
this.timeout(15000);
expect(defaultColl, "JobCollection constructor failed").to.be.an.instanceOf(JobCollection);
expect(defaultColl.root, "default root isn't 'queue'").to.equal("queue");

Expand Down Expand Up @@ -61,10 +62,12 @@ describe("JobCollection", function () {
});

it("should set permissions to allow admin on ClientTest", function () {
this.timeout(15000);
expect(clientTestColl.allows.admin[0]()).to.equal(true);
});

it("should set polling interval", function () {
this.timeout(15000);
let { interval } = clientTestColl;
clientTestColl.promote(250);
expect(interval, "clientTestColl interval not updated").to.not.equal(clientTestColl.interval);
Expand All @@ -75,6 +78,7 @@ describe("JobCollection", function () {
});

it("should run startJobServer on new job collection", function (done) {
this.timeout(15000);
testColl.startJobServer(function (err, res) {
if (err) { done(err); }
expect(res, "startJobServer failed in callback result").to.equal(true);
Expand All @@ -87,6 +91,7 @@ describe("JobCollection", function () {

if (Meteor.isServer) {
it("should create a server-side job and see that it is added to the collection and runs", function (done) {
this.timeout(15000);
const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`;
const job = new Job(testColl, jobType, { some: "data" });
assert.ok(validJobDoc(job.doc));
Expand All @@ -110,6 +115,7 @@ describe("JobCollection", function () {
}

it("should create a job and see that it is added to the collection and runs", function (done) {
this.timeout(15000);
const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`;
const job = new Job(testColl, jobType, { some: "data" });
assert.ok(validJobDoc(job.doc));
Expand All @@ -128,6 +134,7 @@ describe("JobCollection", function () {
});

it("should create an invalid job and see that errors correctly propagate", function (done) {
this.timeout(15000);
const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`;
const job = new Job(testColl, jobType, { some: "data" });

Expand Down Expand Up @@ -164,6 +171,7 @@ describe("JobCollection", function () {
});

it("should create a job and then make a new doc with its document", function (done) {
this.timeout(15000);
let job;
const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`;
const job2 = new Job(testColl, jobType, { some: "data" });
Expand Down Expand Up @@ -191,6 +199,7 @@ describe("JobCollection", function () {
});

it("should should create a repeating job that returns the _id of the next job", function (done) {
this.timeout(15000);
let counter = 0;
const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`;
const job = new Job(testColl, jobType, { some: "data" }).repeat({ repeats: 1, wait: 250 });
Expand Down Expand Up @@ -229,6 +238,7 @@ describe("JobCollection", function () {
});

it("should have dependent jobs run in the correct order", function (done) {
this.timeout(15000);
const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`;
const job = new Job(testColl, jobType, { order: 1 });
const job2 = new Job(testColl, jobType, { order: 2 });
Expand Down Expand Up @@ -256,6 +266,7 @@ describe("JobCollection", function () {

if (Meteor.isServer) {
it("should dry run of dependency check returns status object", function (done) {
this.timeout(15000);
const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`;
const job = new Job(testColl, jobType, { order: 1 });
const job2 = new Job(testColl, jobType, { order: 2 });
Expand Down Expand Up @@ -290,6 +301,7 @@ describe("JobCollection", function () {
}

it("should have dependent job saved after completion of antecedent still runs", function (done) {
this.timeout(15000);
const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`;
const job = new Job(testColl, jobType, { order: 1 });
const job2 = new Job(testColl, jobType, { order: 2 });
Expand All @@ -316,6 +328,7 @@ describe("JobCollection", function () {
});

it("should have dependent job saved after failure of antecedent is cancelled", function (done) {
this.timeout(15000);
const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`;
const job = new Job(testColl, jobType, { order: 1 });
const job2 = new Job(testColl, jobType, { order: 2 });
Expand All @@ -339,6 +352,7 @@ describe("JobCollection", function () {
});

it("should have dependent job saved after cancelled antecedent is also cancelled", function (done) {
this.timeout(15000);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems an odd thing to add to this PR. And just a little odd in general. Is that on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The changes I made seem to have changed the order in which Meteor runs the tests, and a bunch of them started consistently timing out on my machine and PR. I think the timeouts happen for whatever tests run first, because often the startup code (mostly migrations) is still running and ends up slowing down how fast the db ops in tests happen.

It seems like Meteor should have some better solution, like allowing us to indicate when startup code is done, and then starting the test run. Maybe if we moved the migrations and other things into Meteor.startup blocks? But anyway, bumping up the timeouts on every test that failed seemed the quickest way to unblock this.

const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`;
const job = new Job(testColl, jobType, { order: 1 });
const job2 = new Job(testColl, jobType, { order: 2 });
Expand All @@ -359,6 +373,7 @@ describe("JobCollection", function () {
});

it("should have dependent job saved after removed antecedent is cancelled", function (done) {
this.timeout(15000);
const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`;
const job = new Job(testColl, jobType, { order: 1 });
const job2 = new Job(testColl, jobType, { order: 2 });
Expand All @@ -383,6 +398,7 @@ describe("JobCollection", function () {
});

it("should cancel succeeds for job without deps, with using option dependents: false", function (done) {
this.timeout(15000);
const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`;
const job = new Job(testColl, jobType, {});
job.save(function (err2, res2) {
Expand All @@ -397,6 +413,7 @@ describe("JobCollection", function () {
});

it("should have dependent job with delayDeps is delayed", function (done) {
this.timeout(15000);
const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`;
const job = new Job(testColl, jobType, { order: 1 });
const job2 = new Job(testColl, jobType, { order: 2 });
Expand All @@ -423,9 +440,10 @@ describe("JobCollection", function () {
});
});
});
}).timeout(4000);
});

it("should be dependent job with delayDeps is delayed", function (done) {
this.timeout(15000);
const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`;
const job = new Job(testColl, jobType, { order: 1 });
const job2 = new Job(testColl, jobType, { order: 2 });
Expand All @@ -452,9 +470,10 @@ describe("JobCollection", function () {
});
});
});
}).timeout(4000);
});

it("Job priority is respected", function (done) {
this.timeout(15000);
let counter = 0;
const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`;
const jobs = [];
Expand Down Expand Up @@ -486,6 +505,7 @@ describe("JobCollection", function () {
});

it("Job priority is respected", function (done) {
this.timeout(15000);
let counter = 0;
const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`;
const jobs = [];
Expand Down Expand Up @@ -517,6 +537,7 @@ describe("JobCollection", function () {
});

it("A forever retrying job can be scheduled and run", function (done) {
this.timeout(15000);
let counter = 0;
const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`;
const job = new Job(testColl, jobType, { some: "data" }).retry({ retries: testColl.forever, wait: 0 });
Expand All @@ -539,6 +560,7 @@ describe("JobCollection", function () {
});

it("Retrying job with exponential backoff", function (done) {
this.timeout(15000);
let counter = 0;
const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`;
const job = new Job(testColl, jobType, { some: "data" }).retry({ retries: 2, wait: 200, backoff: "exponential" });
Expand All @@ -561,6 +583,7 @@ describe("JobCollection", function () {
});

it("should have a forever retrying job with 'until'", function (done) {
this.timeout(15000);
const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`;
const job = new Job(testColl, jobType, { some: "data" }).retry({ until: new Date(new Date().valueOf() + 1500), wait: 500 });
job.save(function (err, res) {
Expand All @@ -580,9 +603,10 @@ describe("JobCollection", function () {
2500
);
});
}).timeout(5000);
});

it("should autofail and retry a job", function (done) {
this.timeout(15000);
let counter = 0;
const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`;
const job = new Job(testColl, jobType, { some: "data" }).retry({ retries: 2, wait: 0 });
Expand All @@ -608,10 +632,11 @@ describe("JobCollection", function () {
2500
);
});
}).timeout(5000);
});

if (Meteor.isServer) {
it("should save, cancel, restart, refresh: retries are correct.", function (done) {
this.timeout(15000);
const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`;
const j = new Job(testColl, jobType, { foo: "bar" });
j.save();
Expand All @@ -623,6 +648,7 @@ describe("JobCollection", function () {
});

it("should add, cancel and remove a large number of jobs", function (done) {
this.timeout(15000);
const count = 500;
let c = count;
const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`;
Expand Down Expand Up @@ -656,9 +682,10 @@ describe("JobCollection", function () {
}
return result;
})();
}).timeout(5000);
});

it("should have a forever repeating job with 'schedule' and 'until'", function (done) {
this.timeout(15000);
let counter = 0;
const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`;
const job = new Job(testColl, jobType, { some: "data" })
Expand Down Expand Up @@ -695,10 +722,11 @@ describe("JobCollection", function () {
ev = testColl.events.on("jobDone", listener);
return ev;
});
}).timeout(4000);
});
}

it("should run shutdownJobServer on the job collection", function (done) {
this.timeout(15000);
testColl.shutdownJobServer({ timeout: 1 }, function (err, res) {
if (err) { done(err); }
expect(res, true, "shutdownJobServer failed in callback result");
Expand All @@ -711,6 +739,7 @@ describe("JobCollection", function () {

if (Meteor.isClient) {
it("should run startJobServer on remote job collection", function (done) {
this.timeout(15000);
remoteServerTestColl.startJobServer(function (err, res) {
if (err) { done(err); }
expect(res, "startJobServer failed in callback result").to.equal(true);
Expand All @@ -719,6 +748,7 @@ describe("JobCollection", function () {
});

it("should create a job and see that it is added to a remote server collection and runs", function (done) {
this.timeout(15000);
const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`;
const job = new Job(remoteServerTestColl, jobType, { some: "data" });
assert.ok(validJobDoc(job.doc));
Expand All @@ -735,6 +765,7 @@ describe("JobCollection", function () {
});

it("should run shutdownJobServer on remote job collection", function (done) {
this.timeout(15000);
remoteServerTestColl.shutdownJobServer({ timeout: 1 }, function (err, res) {
if (err) { done(err); }
expect(res, "shutdownJobServer failed in callback result").to.equal(true);
Expand Down
2 changes: 1 addition & 1 deletion server/imports/fixtures/shops.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ const shop = {
enabled: true
}],
workflow: {
status: "active"
status: "new"
},
public: true,
brandAssets: [
Expand Down
3 changes: 3 additions & 0 deletions server/methods/core/shops.app-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,12 +98,14 @@ describe("core shop methods", function () {
});

it("creates a new shop for admin for userId and a partial shopObject", function () {
this.timeout(15000);
const partialShop = { name };

Meteor.call("shop/createShop", userId, partialShop);
});

it("creates a new shop for admin for userId and a partial shopObject ignoring extraneous data", function () {
this.timeout(15000);
const extraneousData = Random.id();
const partialShop = { name, extraneousData };

Expand All @@ -120,6 +122,7 @@ describe("core shop methods", function () {

describe("shop/changeLayouts", function () {
it("should replace every layout with the new layout", function () {
this.timeout(15000);
const shop = Factory.create("shop");
Meteor.call("shop/changeLayouts", shop._id, "myNewLayout");
const myShop = Shops.findOne(shop._id);
Expand Down
Loading