t/1: Introduce tests and Bender.js #11
Conversation
…o it can be tested.
fredck
added
the
type:improvement
| ( function() { | ||
| // Override bender.start to wait until the "ckeditor" module is ready. | ||
| var originalStart = bender.start; | ||
| bender.start = function() { |
There was a problem hiding this comment.
I'm not sure if we discussed this already, but I think we should consider adding some space between the variable definition and the rest of the code. It would improve the readability of the code because right now it looks a bit crammed.
There was a problem hiding this comment.
+1 and in this case it also matches the "margin around blocks" rule that we agreed on.
… match with "it".
|
@gregpabian, I've fixed all your comments. |
|
|
||
| expect( path ).to.equal( basePath + 'plugins/test/' ); | ||
|
|
||
| CKEDITOR.getPluginPath = originalGetPluginPath; |
There was a problem hiding this comment.
It's not going to be executed if expect().equal() throws. I don't know how it works in Mocha+Chai, but in CKE4 you would need to handle teardown outside the test or in try-catch-finally :D.
There was a problem hiding this comment.
Nice catch... I've just pushed a fix for it by simply reverting the override before the assertion.
|
Just pushed commits that introduce |
|
|
||
| bender.amd.require( 'ckeditor', 'ckeditor-core' ); | ||
|
|
||
| describe( 'getPluginPath()', function( CKEDITOR, core ) { |
There was a problem hiding this comment.
I'm not so convinced with this solution - this means we have to keep in mind the order of required modules and write them down in every describe's callback. I think this solution would work better:
var modules = bender.amd.require( 'ckeditor', 'ckeditor-core' );And then we can use modules.ckeditor etc.
Besides, I feel it's too closely coupled with Mocha (which could be avoided thanks to bender.defer).
There was a problem hiding this comment.
Still you would much probably not call modules.ckeditor directly, but instead have var CKEDITOR = modules.ckeditor on every single test, messing things again. This describe() helper cleans things up by defining this variables once only as its callback parameters.
Also, to make it work right, var modules would have to go in the global scope. Maybe not a big deal, but not better than the current solution as well.
Additionally, the proposed gives this sensation of being much similar to AMD (list of requires and modules as params).
I don't know, I still like it a lot and I don't see the big deal.
Besides, I feel it's too closely coupled with Mocha (which could be avoided thanks to bender.defer).
I don't understand this.
There was a problem hiding this comment.
this means we have to keep in mind the order of required modules and write them down in every describe's callback
Hm... Makes sense. Usually we're testing one thing in one file, so there will be one describe() call. But eventually this mechanism will be adopted by benderjs-amd and then other projects will start using it and may want to use far more describe()s.
Besides that, I'm ok with both APIs.
There was a problem hiding this comment.
But eventually this mechanism will be adopted by benderjs-amd
Just to underline this, this feature has been coded with the CKEditor 5 dev environment in mind, exclusively, and is proposed in the ckeditor5 plugin for bender right because of this. It is optimized for our needs and the way we code tests.
There was a problem hiding this comment.
I don't understand this.
I mean the current solution is for BDD frameworks only (it's based on describe() method) - you can't use other testing framework/style with it, whereas the solution with the modules variable would work.
There was a problem hiding this comment.
I agree, but please check my previous comment. This is intentional.
A generic solution may look different, in fact, but we should not miss the chance to bring an optimized solution to be used by us only.
|
Changed strategy so now |
| ]; | ||
|
|
||
| module.exports = { | ||
| name: 'bender-framework-ckeditor5', |
There was a problem hiding this comment.
framework is no longer needed
t/1: Introduce tests and Bender.js
| @@ -9,3 +9,4 @@ | |||
|
|
|||
| node_modules/** | |||
| build/** | |||
| coverage/** | |||
There was a problem hiding this comment.
What's in this directory? AFAICS we didn't have to ignore it in CKE4.
There was a problem hiding this comment.
This is created while running bender run with coverage plugin enabled. I want to avoid a situation where someone accidentally pushes CC results to the repo.
Fixes #1.
Tests for ckeditor5 code included with 100% code coverage.