Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

t/1: Introduce tests and Bender.js #11

Merged
merged 32 commits into from
Feb 12, 2015
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
118a4eb
Added npm devDependency for bender with mocha and chai.
fredck Jan 9, 2015
ffe17c5
Initialized the bender configuration file.
fredck Jan 9, 2015
e7e9834
Introduced the "ckeditor" application in bender and the CKEditor 5 pl…
fredck Jan 16, 2015
09fa91e
Added code coverage support for bender.
fredck Jan 20, 2015
c9ad4d0
Added basic tests for AMD.
fredck Jan 16, 2015
5aa7d5a
Made it in a way that "ckeditor-core" keeps its original definition s…
fredck Jan 20, 2015
dd79ce5
Removed code that should (hopefully) be deprecated as scripts always …
fredck Jan 20, 2015
2946304
Added a "dev" flag to CKEDITOR.
fredck Jan 20, 2015
47cde86
Added tests for CKEDITOR.
fredck Jan 20, 2015
87f8200
Changed the ckeditor plugin for bender to user bender.defer().
fredck Jan 21, 2015
19e29d0
Reviewed the tests titles and made better use of "describe" to better…
fredck Jan 21, 2015
6488116
Standardized the equality checks to `to.equal()`.
fredck Jan 21, 2015
0c63b0a
Fixed a wrong object check in tests.
fredck Jan 21, 2015
ebcbd4e
Minor white-space cleanup.
fredck Jan 21, 2015
758b958
Fixed an override reversion that would not take place in case of test…
fredck Jan 21, 2015
26f6c7c
Introduced bender.amd.require() - a tool to easily require CKEditor m…
fredck Jan 22, 2015
41ec3ce
Reviewed tests to use bender.amd.require().
fredck Jan 22, 2015
70373f7
Avoided a test dependency on utils.isObject(), which is not in use an…
fredck Jan 22, 2015
1137ff7
Added support for describe.skip().
fredck Jan 22, 2015
a7f512d
Revert "Added support for describe.skip()."
fredck Jan 22, 2015
907ef34
Revert "Reviewed tests to use bender.amd.require()."
fredck Jan 22, 2015
af63433
Revert "Introduced bender.amd.require() - a tool to easily require CK…
fredck Jan 22, 2015
a8afa0a
Re-introduced bender.amd.require() - a tool to easily require CKEdito…
fredck Jan 22, 2015
338f468
Re-reviewed tests to use bender.amd.require().
fredck Jan 22, 2015
c15dbcd
Moved the bender.amd.require() call out of describe().
fredck Jan 22, 2015
938c8d9
Removing third-party libs from code coverage.
fredck Jan 22, 2015
6bf8aa1
Passing code coverage through untestable code.
fredck Jan 22, 2015
61c5f9c
Upgraded to Bender 0.2.
fredck Feb 3, 2015
0de6f6b
Corrected the plugin name.
fredck Feb 3, 2015
a60cb57
Added support for Sinon.
fredck Feb 5, 2015
a530d20
Exclude code coverage reports from the repository.
gregpabian Feb 12, 2015
0968169
Minor typo fixes.
gregpabian Feb 12, 2015
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions bender.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/* jshint browser: false, node: true */

'use strict';

var config = {
plugins: [
// Uncomment to enable code coverage.
// 'benderjs-coverage',

'benderjs-mocha',
'benderjs-chai',
'dev/bender/plugins/ckeditor5'
],

framework: 'mocha',

applications: {
ckeditor: {
path: '.',
files: [
'node_modules/requirejs/require.js',
'ckeditor.js'
]
}
},

tests: {
all: {
applications: [ 'ckeditor' ],
paths: [
'tests/**',
'node_modules/ckeditor5-*/tests/**'
]
}
},

coverage: {
paths: [
'ckeditor.js',
'src/**/*.js',
'node_modules/ckeditor5-*/src/**/*.js'
]
}
};

module.exports = config;
15 changes: 3 additions & 12 deletions ckeditor.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* For licensing, see LICENSE.md.
*/

/* global requirejs, define, require, window, document, location */
/* global requirejs, define, require, window, document */

'use strict';

Expand Down Expand Up @@ -73,10 +73,9 @@

// Define a new "ckeditor" module, which overrides the core one with the above and the dev stuff.
define( 'ckeditor', [ 'ckeditor-core', 'ckeditor-dev', 'utils' ], function( core, dev, utils ) {
utils.extend( core, root.CKEDITOR, ( dev || {} ) );
root.CKEDITOR = core;
root.CKEDITOR = utils.extend( {}, core, root.CKEDITOR, ( dev || {} ) );

return core;
return root.CKEDITOR;
} );

function getBasePath() {
Expand All @@ -99,14 +98,6 @@
}
} );

if ( path.indexOf( ':/' ) == -1 && path.slice( 0, 2 ) != '//' ) {
if ( path[ 0 ] == '/' ) {
path = location.href.match( /^.*?:\/\/[^\/]*/ )[ 0 ] + path;
} else {
path = location.href.match( /^[^\?]*\/(?:)/ )[ 0 ] + path;
}
}

return path;
}
} )( window );
20 changes: 20 additions & 0 deletions dev/bender/plugins/ckeditor5/lib/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/**
* @license Copyright (c) 2003-2015, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md.
*/

/* jshint node: true */

'use strict';

var path = require( 'path' );
var files = [
path.join( __dirname, '../static/extensions.js' )
];

module.exports = {
name: 'bender-framework-ckeditor5',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

framework is no longer needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed with 0de6f6b.


files: files,
include: files
};
5 changes: 5 additions & 0 deletions dev/bender/plugins/ckeditor5/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"name": "benderjs-ckeditor5",
"description": "CKEditor 5 plugin for Bender.js",
"main": "lib/index.js"
}
18 changes: 18 additions & 0 deletions dev/bender/plugins/ckeditor5/static/extensions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/**
* @license Copyright (c) 2003-2015, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md.
*/

/* globals bender, CKEDITOR */

'use strict';

( function() {
// Override bender.start to wait until the "ckeditor" module is ready.
var originalStart = bender.start;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not bender.defer()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just because defer() wasn't still available when I wrote it :) I'll change it.

bender.start = function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 and in this case it also matches the "margin around blocks" rule that we agreed on.

CKEDITOR.require( [ 'ckeditor' ], function() {
originalStart.apply( this, arguments );
} );
};
} )();
4 changes: 4 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@
"del": "~1.1",
"ncp": "~1.0",
"replace": "~0.3.0",
"benderjs": "~0.1",
"benderjs-mocha": "~0.1",
"benderjs-chai": "~0.1.1",
"benderjs-coverage": "~0.1.1",
"grunt": "~0",
"grunt-jscs": "~1",
"grunt-contrib-jshint": "~0",
Expand Down
8 changes: 8 additions & 0 deletions src/ckeditor-dev.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,14 @@

define( 'ckeditor-dev', function() {
return {
/**
* A flag specifying whether CKEditor is running in development mode (original source code).
*
* This property is not defined in production (compiled, build code).
*/
isDev: true,

// Documented in ckeditor-core/ckeditor.
getPluginPath: function( name ) {
return this.basePath + 'node_modules/ckeditor-plugin-' + name + '/src/';
}
Expand Down
31 changes: 31 additions & 0 deletions tests/amd/amd.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/**
* @license Copyright (c) 2003-2015, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md.
*/

/* globals describe, it, expect, CKEDITOR */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're gonna have describe, it and expect in every test, maybe we can "globalize" them in a single place, like .jshintrc file in the tests/ directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i tried this already but it doesn't work in the way we're using jshint right now (by pointing to a specific configuration file).

If we would like to do it it looks like we would have to bloat (further) the root by adding . jshintrc there (and at this point .jscs) as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh... I've been already thinking about this too. I think that the easiest (thus, doable) solutions are:

  • Define little in the global configs and use comments extensively. The thing is that the global config of the ckeditor5-core module should for example contain CKEDITOR because it should be available in most of the files, but global config of the ckeditor5 module does not have to, because many files in this repository are related to grunt tasks. The rest will need to be defined in comments, which is not that bad, because you only need to do this when you start using some variable.
  • Define a lot in the global configs. We can add most of the stuff that can be available somewhere in tests or dev code to the global config and we won't need to add so many comments. This makes the check less precise, but it's still good.

In CKEditor 4 we use now the second approach and it's ok. For instance, you never use it or describe in the dev code, so you don't do mistakes related to them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gonna have an issue for this once this PR gets merged.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reported #13.


'use strict';

CKEDITOR.define( 'testModule', [ 'ckeditor' ], function( ckeditor ) {
return {
test: ( typeof ckeditor == 'object' )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typeof null == 'object'

Touché! :trollface:

};
} );

describe( 'amd', function() {
it( 'require() should work with defined module', function( done ) {
CKEDITOR.require( [ 'testModule' ], function( testModule ) {
expect( testModule ).to.have.property( 'test' ).to.be.true();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary empty line (I know I'm bit*hing right now, but I think this should be consistent across our tests).

done();
} );
} );

it( 'require() should work with core module', function( done ) {
CKEDITOR.require( [ 'utils' ], function( utils ) {
expect( utils ).to.be.an( 'object' );
done();
} );
} );
} );
70 changes: 70 additions & 0 deletions tests/ckeditor/basepath.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
/**
* @license Copyright (c) 2003-2015, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md.
*/

/* globals beforeEach, describe, it, expect, CKEDITOR, window, document */

'use strict';

beforeEach( function() {
// Ensure that no CKEDITOR_BASEPATH global is available.
delete window.CKEDITOR_BASEPATH;

// Remove all script elements from the document.
removeScripts();
} );

describe( 'ckeditor.basePath', function() {
it( 'Full URL', function( done ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name of the test case is obscure. It should be written using natural language, e.g. "Full URL should do something".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review all the test case titles - I don't want to comment every single one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm reviewing them right now and in fact it makes much more sense. We must also get used to use describe() properly.

CKEDITOR.require( [ 'ckeditor' ], function( CKEDITOR ) {
addScript( 'http://bar.com/ckeditor/ckeditor.js' );
expect( CKEDITOR._getBasePath() ).equals( 'http://bar.com/ckeditor/' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

).to.equal( would read better

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, in fact there are many possible variations: to.equal(), equal(), equals().

We may agree on best practices, but in any case I think we should not enforce this because it'll be a pain in the ass for contributors to have their stuff rejected because of such things, considering that chai gives all these possibilities and they're all right.

I'll change what we have now to to.equal().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, which is better: to.equal() or to.be.equal()?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The one that is grammatically correct :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember we've been talking about using the shortest (or reasonably short) versions. Apart from grammatical correctness, I wouldn't like to use to.be.equal() because it is terrible to write. Personally, I would keep expect().equals().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.to.equal() is just two letters longer but reads way better, just sayin'...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually to make it right, it should be to.be.equal.to()... too long!!!

to.equal is grammatically right as well, so let's use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Reinmar, we use ().to a lot for other cases so this would just get similar to others.

I'll make the change and lets see how it looks like.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.to.equal() is just two letters longer but reads way better, just sayin'...

I know, but while writing my comment I noticed that it's not going through my fingers too well :P

Anyway - to.equal() is not bad. I'll perhaps use to type it so it will be faster. Just saying so we remember to keep them short.

done();
} );
} );

it( 'CKEDITOR_BASEPATH', function( done ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

CKEDITOR.require( [ 'ckeditor' ], function( CKEDITOR ) {
window.CKEDITOR_BASEPATH = 'http://foo.com/ckeditor/';
expect( CKEDITOR._getBasePath() ).equals( 'http://foo.com/ckeditor/' );
done();
} );
} );

it( 'Ensure that no browser keep script URLs absolute or relative', function( done ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it( 'should ensure that...

// Browsers should convert absolute and relative URLs to full URLs.
// If this test fails in any browser, _getBasePath() must be reviewed to deal with such case (v4 does it).

test( '/absolute/url/ckeditor.js' );
test( '../relative/url/ckeditor.js' );

done();

function test( url ) {
removeScripts();

var script = addScript( url );

// Test if the src now contains '://'.
expect( /:\/\//.test( script.src ) ).to.be.true();
}
} );
} );

function addScript( url ) {
var script = document.createElement( 'script' );
script.src = url;
document.head.appendChild( script );

return script;
}

function removeScripts() {
var scripts = [].slice.call( document.getElementsByTagName( 'script' ) );
var script;

while ( ( script = scripts.shift() ) ) {
script.parentNode.removeChild( script );
}
}
43 changes: 43 additions & 0 deletions tests/ckeditor/ckeditor.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/**
* @license Copyright (c) 2003-2015, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md.
*/

/* globals describe, it, expect, CKEDITOR */

'use strict';

describe( 'ckeditor', function() {
it( 'getPluginPath() with development source code', function( done ) {
CKEDITOR.require( [ 'ckeditor' ], function( CKEDITOR ) {
var basePath = CKEDITOR.basePath;
var path = CKEDITOR.getPluginPath( 'test' );

if ( CKEDITOR.isDev ) {
expect( path ).equals( basePath + 'node_modules/ckeditor-plugin-test/src/' );
} else {
expect( path ).equals( basePath + 'plugins/test/' );
}
done();
} );
} );

it( 'getPluginPath() with production code', function( done ) {
CKEDITOR.require( [ 'ckeditor', 'ckeditor-core' ], function( CKEDITOR, core ) {
// To be able to run this test on both dev and production code, we need to override getPluginPath with the
// core version of it and restore it after testing.
var originalGetPluginPath = CKEDITOR.getPluginPath;
CKEDITOR.getPluginPath = core.getPluginPath;

// This test is good for both the development and production codes.
var basePath = CKEDITOR.basePath;
var path = CKEDITOR.getPluginPath( 'test' );

expect( path ).equals( basePath + 'plugins/test/' );

CKEDITOR.getPluginPath = originalGetPluginPath;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch... I've just pushed a fix for it by simply reverting the override before the assertion.


done();
} );
} );
} );