morebits: convert Morebits.wiki.page to use promises #911
Conversation
|
I at last figured out a nice scheme for doing unit testing. Will add the tests in a few days or weeks. Suggest not reviewing this until then, as this would be a lot easier to review then. |
|
Neat! I've been mulling some tests for |
siddharthvp
force-pushed
the
page-promise
branch
from
150af56
to
21a082e
Compare
|
Ok, I've now added the unit tests. How to run:
Do this on the testwiki, though the tests which create pages also do cleanup after themselves by deleting the pages afterwards. Others edit the user's sandbox. I've created Wikipedia:Twinkle/ExampleNonProtectedPage and Wikipedia:Twinkle/ExampleProtectedPage used in a couple of tests. We are using browser-based unit testing with the content of workspace files made available in the browser through the localhost server. Any attempt to do node-based unit testing would require trying to replicate the MW environ (mw.config etc etc) which is extremely difficult. Mocha is used as the testrunner and chai as the assertion library, loaded over CDNs for now. We might as well keep a copies in the repo and load using the server. All 33 of the tests are passing. Yippee! |
siddharthvp
force-pushed
the
page-promise
branch
from
21a082e
to
bc56c55
Compare
|
Really cool! I'm not particularly familiar with mocha, but this looks pretty great to me. I want to get these out next month, so I'll comment more then, but love it. I can say from personal experience that trying to replicate the MW environ (mw.config etc etc) is indeed extremely difficult. 😭 😆 😭 |
siddharthvp
force-pushed
the
page-promise
branch
2 times, most recently
from
d1c6f0a
to
80d1890
Compare
Amorymeltzer
force-pushed
the
page-promise
branch
from
80d1890
to
cb9c0ff
Compare
… callback arguments
Amorymeltzer
force-pushed
the
page-promise
branch
from
cb9c0ff
to
c165df2
Compare
|
Rebased, merged, fixed some errors |
There was a problem hiding this comment.
@siddharthvp I don't know what to say on this except it is really very excellent. Promises are probably the hardest thing to wrap my head around, all the more so via jQuery, so I'm really very impressed. The testing regime is great AFAICT, and is a really great start. I'd really like to go further down this road and flesh it out, this is a really nice fire to get going with! And wow does it speed things up! We might need to finally do some purging (e.g. #364) to play catch up.
Unfortunately, I don't think I can merge this. It suffers pretty heavily from the same issue as #1036, where it can redirect to //wiki//wiki//wiki/PAGENAME. Maybe the intersection will help debug both of them, or at least lead us to figure out how to rewrite Morebits.wiki.actionCompleted. Presumably related, but a single tagging can lead to several pairs of "Tagging completed" messages (e.g. three pairs).
| ctx.statusElement.error('Internal error: attempt to save a page that has not been loaded!'); | ||
| ctx.onSaveFailure(this); | ||
| return; | ||
| return returnError.call(this, 'int-notloaded', 'Internal error: attempt to save a page that has not been loaded!'); |
There was a problem hiding this comment.
Guessing this stands for "internal"? Unimportant to do here, but down the line I think it'd do better with "morebits-" or at least "mb-", then eventually document them somewhere.
| // We don't really care about the response | ||
| wikipedia_api.post(); | ||
| } else { | ||
| return $.Deferred().rejectWith(this, [this, 'missingparam', 'No rcid available']); |
There was a problem hiding this comment.
Why do we need to hardcode the errors here? Are they not accessible?
The server.js file from wikimedia-gadgets#911. No dependencies. While it's possible to set up a server using `php -S` and `python -m SimpleHTTPServer` etc, putting it in package.json means IDEs can provide a one-click button for running it.
The server.js file from #911. No dependencies. While it's possible to set up a server using `php -S` and `python -m SimpleHTTPServer` etc, putting it in package.json means IDEs can provide a one-click button for running it.
Needed for taskManager to know when the logging has finished in promise-based rewrites. Resolves the kludge in twinkleprod. Of course this would have been much simpler if Morebits.wiki.page methods themselves returned promises (wikimedia-gadgets#911).
|
This is hopeless. I haven't really tested again, but it seems the way to go ahead with this is to not use Morebits.wiki.actionCompleted.* The alternative to that is Morebits.taskManager(), which requires tasks to return promises in the first place. Looks like this can be closed, unless @Amorymeltzer you have ideas on getting |
Includes the commits of pre-requisite #765.
Closes #783.
All functions in Morebits.wiki.page are updated to support promises. The existing callback scheme continues to work. Also, fixes the issues with inconsistency in callback arguments as mentioned in #783.
It's worth noting that after @Amorymeltzer's #712, there is a remarkable structural similarity across most functions.
The
stabilizefunction is untested, as FlaggedRevs is not there on the testwiki, so I have no way to test it.I've added "tests" as morebits-test-promises.js (last commit). These tests aren't automated - you've copy paste each code chunk to the console to see that that chunk is working as mentioned. Suitable for running on the testwiki only. Converting these to unit tests shouldn't be that difficult, though I couldn't get it right in an hour and a half.Unit tests added, see #911 (comment)