refactor: use native promises and drop q #720
Conversation
migrated away from q, and used native promises. unhandled rejections in unit tests now cause the test runner to fail. ensured all rejections are ignored with no-op fn. found and fixed several broken tests that did not report failures correctly. fixed wait() not used correctly outside .then, as it's returning a higher order fn rather than the promise (to chain values in .then callbacks) implemented a tiny test polyfill for Promise.allSettled to support older Node versions. worked around a timezone issue and a test failing when running it in ~1am locally, heh. added ability to use .only/skip on the test proxy of mocha's describe fn
| it("should allow listing resources specifying direction", async function () { | ||
| this.timeout(TIMEOUT.LONG); | ||
| Q.all( | ||
| await Promise.all([ |
| ).then(function (resource) { | ||
| expect(resource).not.to.eql(void 0); | ||
| console.log(resource); | ||
| // console.log(resource); |
| it("should return usage values for a specific date", function () { | ||
| const yesterday = formatDate(subDate(new Date(), {days: 1}), "dd-MM-yyyy"); | ||
| return cloudinary.v2.api.usage({date: yesterday}) | ||
| const twoDaysAgo = formatDate(subDate(new Date(), {days: 2}), "dd-MM-yyyy"); |
| arg => (arg != null ? arg.query : void 0) === "moderations=true", "moderations=true" | ||
| )); | ||
| })); | ||
| callReusableTest("a list with a cursor", cloudinary.v2.api.resources_by_moderation, "manual", "approved"); |
There was a problem hiding this comment.
moved this outside the it(), as it defines another test
| it("should support listing by moderation kind and value", async function () { | ||
| for (const stat of ["approved", "pending", "rejected"]) { | ||
| // eslint-disable-next-line no-await-in-loop | ||
| await helper.provideMockObjects(async (mockXHR, writeSpy, requestSpy) => { |
There was a problem hiding this comment.
made sure every call has a separate set of spies
| await cloudinary.v2.api.update(result.public_id, { | ||
| unique_display_name: true | ||
| }) | ||
| sinon.assert.calledWith(writeSpy, sinon.match(helper.apiParamMatcher('unique_display_name', true, "unique_display_name=true"))); |
There was a problem hiding this comment.
fixed test. Promise was never propagated to mocha.
api.update uses a POST call, so parameter is passed in write, not query param
| it("should send ftp:// URLs to server", function () { | ||
| cloudinary.v2.uploader.upload("ftp://example.com/1.jpg", { | ||
| it("should send ftp:// URLs to server", async function () { | ||
| await cloudinary.v2.uploader.upload("ftp://test/1.jpg", { |
There was a problem hiding this comment.
the domain caused the Promise to hang
| chunk_size: 40000, | ||
| tags: UPLOAD_TAGS | ||
| tags: UPLOAD_TAGS, | ||
| disable_promises: true |
There was a problem hiding this comment.
this was missing and was causing an unhandled rejection
| stat = fs.statSync(LARGE_VIDEO); | ||
| expect(stat).to.be.ok(); | ||
| return Q.denodeify(cloudinary.v2.uploader.upload_chunked)(LARGE_VIDEO, { | ||
| cloudinary.v2.uploader.upload_chunked(LARGE_VIDEO, { |
There was a problem hiding this comment.
used callback style call in test, like other ones
| it("should update timestamp for each chunk", function (done) { | ||
| var writeSpy = sinon.spy(ClientRequest.prototype, 'write'); | ||
| return Q.denodeify(cloudinary.v2.uploader.upload_chunked)(LARGE_VIDEO, { | ||
| cloudinary.v2.uploader.upload_chunked(LARGE_VIDEO, { |
|
|
||
| it("should reject with promise rejection if disable_promises: false", function (done) { | ||
| const spy = sinon.spy(); | ||
| const unhandledRejectionSpy = sinon.spy(); |
There was a problem hiding this comment.
simplified by using spy as listener directly.
ensured listener is added before the upload_large call
|
|
||
| it("should reject with promise rejection by default", function (done) { | ||
| const spy = sinon.spy(); | ||
| const unhandledRejectionSpy = sinon.spy(); |
| it("should reject without promise rejection if disable_promises: true", function (done) { | ||
| const spy = sinon.spy(); | ||
|
|
||
| const unhandledRejectionSpy = sinon.spy(); |
| expect(firstUpload.metadata[smdNumberField]).to.eql(123); | ||
| expect(secondUpload.metadata[smdNumberField]).to.eql(123); | ||
| }); | ||
| type: 'integer' |
There was a problem hiding this comment.
this was a test bug. Promise was never returned to mocha, so it never knew about the failures
|
|
||
| // eslint-disable-next-line no-await-in-loop | ||
| await wait(delay) | ||
| await wait(delay)(); |
There was a problem hiding this comment.
this was a bug. wait is not sleep. it returns a fn, not the Promise directly
| }); | ||
| } | ||
|
|
||
| makeSuite.only = describe.only; |
There was a problem hiding this comment.
can now .only/skip on describe calls
| @return {Promise} | ||
| */ | ||
| exports.provideMockObjects = function (providedFunction) { | ||
| exports.provideMockObjects = async function (providedFunction) { |
|
FYI @cloudinary-pkoniu @aleksandar-cloudinary 4 tests in uploader->sprite fail on my local machine for both |
|
btw, one of the benefits to waiting those rejections in unit tests is being able to use async-await in implementation now... previously, this would have introduced implicit asynchronicity that was not handled by tests, but now it's safer to use such cleaner patterns. not part of this PR, but possible future improvement to this library. |
1 participant
migrated away from q, and used native promises.
unhandled rejections in unit tests now cause the test runner to fail. ensured all rejections are ignored with no-op fn.
found and fixed several broken tests that did not report failures correctly.
fixed wait() not used correctly outside .then, as it's returning a higher order fn rather than the promise (to chain values in .then callbacks)
implemented a tiny test polyfill for Promise.allSettled to support older Node versions.
worked around a timezone issue and a test failing when running it in ~1am locally, heh.
added ability to use .only/skip on the test proxy of mocha's describe fn
closes #711
closes #686