Ghost: Refactor tests to return Promises when possible

[acburdine]

Throughout Ghost's test suite there are many places where the done() async callback is used to handle asynchronous behavior. There are also many places where the test function returns a promise.

As part of #7165 the test cases that returned promises explicitly were refactored to _not_ use the done() callback to handle behavior. (Mocha 3 throws an exception if you use both).

However, there are still lots of places throughout the tests that still use the done() callback because the test function doesn't return a Promise when it should.

To close this issue, all of the tests cases need to be evaluated to see if they can return a Promise. If so, then they should be made to, and the done() callback should not be used in those instances.

_Note: there still may be scenarios where the done callback needs to be used (for instance, if the test can't return a promise) However, because most of the test functions that have asynchronous behavior do deal with promises, the need for explicitly using done should be very little_

[janvt]

Taking a stab at this, seems pretty straightforward.

[acburdine]

@janvt there's already a PR open for this in #7254 - you might see if you can help out there :)

[Divya063]

@acburdine Can I work on this?

[ErisDS]

Hey @Divya063,

I have been thinking about this a lot whilst working on our tests recently. Because we are so often testing the opposite case, e.g. checking that it correctly fails, I feel like there are too many cases where we still need to use done. I think this makes the tests harder to read / more confusing.

If you take a look at the previous attempt to do this: https://github.com/TryGhost/Ghost/pull/7254

You see there were some cases that needed to be left, and that deciding which is which isn't easy.

cc @kirrg001 I don't know what you prefer here?

I think it is really not a good use of time and that @Divya063 if you would like to get involved with contributing, a similar but perhaps more useful task would be to work on upgrading should and/or sinon, which have breaking changes to their APIs.

[kirrg001]

Also hey. Sorry for late response, @Divya063

First thanks so much for your interest.

I think it is really not a good use of time and that

I agree with @ErisDS. There are cases where we want to use done or where somebody prefers to use done. There is no need to go over ~2000 test cases and double check whether we use done or return a Promise.

if you would like to get involved with contributing, a similar but perhaps more useful task would be to work on upgrading should and/or sinon, which have breaking changes to their APIs.

If you are interested in upgrading sinon/mocha, please let us know by raising an issue with:

• changes which were introduced in a newer major version
• changes we have to adapt in our code base with one example code snippet
• then @ErisDS or me will reply

Another task i can think of is https://github.com/TryGhost/Ghost/issues/7696, which is a task to dive in into the codebase.

If you have any questions, let us know in the contributing channel in slack.

I am going to close this issue now.

[Divya063]

@ErisDS @kirrg001 Thank you so much for the information, I would like to work on #7696 as well as upgrading sinon/mocha.