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([ |
There was a problem hiding this comment.
was not originally awaited
| ).then(function (resource) { | ||
| expect(resource).not.to.eql(void 0); | ||
| console.log(resource); | ||
| // console.log(resource); |
There was a problem hiding this comment.
removed log in test
| 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) { |
There was a problem hiding this comment.
simplified this one
|
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. |
|
any chance for a review? I feel like this one gets this SDK one step closer to 2025. |
|
Hey @AviVahl! Thank you very much for your contribution, it's much appreciated. Unfortunately it currently lacks a compatibility layer for the Q-specific helpers that some users might still rely on. Without that shim, apps running on the lower bound we advertise (Node 9) blow up as soon as they call .finally()/.done()/.nodeify() on the promises returned by cloudinary.v2.*, so we can’t merge it yet. I’ll put together a small follow-up PR for your fork that wraps the native promises we now create and re-exposes those helpers (plus a Node‑9 regression test). Once you’re comfortable pulling that compatibility layer into your branch, we can continue working on this PR. |
|
Sorry, I didn't realize supporting Node 9 is important to your team. About the q-specific helpers, the entire idea here was to remove this deprecated library and its overhead. You could release a new major of this library if you're worried about people using these helpers. Feel free to take my changes as-is, and adjust them later on, or simply close this PR and I'll delete my fork. |
|
It is fine. Thank you. |
2 participants
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