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/16938 Extract common PFW tests loop. #333

Merged
merged 4 commits into from Apr 6, 2017

Conversation

Projects
None yet
2 participants
@f1ames
Copy link
Member

commented Mar 31, 2017

Proposed merge commit message:
Tests: Common Paste from Word test loop extracted as a helper.

f1ames added some commits Mar 31, 2017

@Comandeer Comandeer self-requested a review Apr 5, 2017

@Comandeer
Copy link
Member

left a comment

The changes simplify code, but I feel we could do even better! ;)

testData = options.testData || { _should: { ignore: {} } },
ignoreAll = options.ignoreAll === true,
compareRawData = options.compareRawData === true,
customFilters = options.customFilters ? options.customFilters : null;

This comment has been minimized.

Copy link
@Comandeer

Comandeer Apr 5, 2017

Member

IMO it can be much simplified by using CKEDITOR.tools.extend to set default values for undefined options.

compareRawData = options.compareRawData === true,
customFilters = options.customFilters ? options.customFilters : null;

_prepareTests( testData, tests, browsers, wordVersions, compareRawData, customFilters, false, ignoreAll );

This comment has been minimized.

Copy link
@Comandeer

Comandeer Apr 5, 2017

Member

And if we're going with simplified handling of options object, we can also pass it straight to _prepareTests instead of passing absurdly high number of parameters.

/* global createTestCase */
/* exported createTestSuite */

// Creates a test suite based on the options provided. The test suite then should be passed to `bender.test` to run tests.

This comment has been minimized.

Copy link
@Comandeer

Comandeer Apr 5, 2017

Member

Why not standard JSDoc comments, similar to our existing tests? Comments from tests don't leak into documentation, so it's safe to use JSDoc in a normal way.

if ( testsKeys.length ) {
for ( i = 0; i < testsKeys.length; i++ ) {
for ( k = 0; k < browsers.length; k++ ) {
for ( j = 0; j < wordVersions.length; j++ ) {

This comment has been minimized.

Copy link
@Comandeer

Comandeer Apr 5, 2017

Member

Shouldn't it be before browsers loop? I mean, j is usually before k… :P


_prepareTests( testData, tests, browsers, wordVersions, compareRawData, customFilters, false, ignoreAll );

_prepareTests( testData, ticketTests, browsers, wordVersions, compareRawData, customFilters, true, ignoreAll );

This comment has been minimized.

Copy link
@Comandeer

Comandeer Apr 5, 2017

Member

If I'm seeing correctly, the only place that uses the information if the tests are ticket ones or not is inside createTestCase to correctly choose the right path to lookup tests. In that case IMO we could drop ticketTests at all and make all tests… well, just tests. The info that these tests are ticket ones could be moved into their name.

I'm not seeing any sense in a special treatment of Tickets subdirectory, especially when we have also some others (e.g. No_heuristics). And this change will simplify test code even more.

testData._should.ignore[ testName ] = true;
}

testData[ testName ] = createTestCase( testKey, wordVersion, browsers[ k ], areTicketsTests, compareRawData, customFilters );

This comment has been minimized.

Copy link
@Comandeer

Comandeer Apr 5, 2017

Member

I'm a big fan of options object! Maybe we could use this approach also in createTestCase? It would be much more readable than 6 parameters.

@@ -1,5 +1,4 @@
/* global assertWordFilter,Q */
/* bender-include: _helpers/pfwTools.js */
/* global assertWordFilter,Q,console */
/* exported createTestCase */

// @param {boolean} [compareRawData=false] If `true` test case will assert against raw paste's `data.dataValue` rather than

This comment has been minimized.

Copy link
@Comandeer

Comandeer Apr 5, 2017

Member

If we're adding docs in other places, then we can also add missing docs for createTestCase as only one parameter is described.

@f1ames f1ames requested a review from Comandeer Apr 6, 2017

@Comandeer Comandeer merged commit c896eaa into ckeditor:major Apr 6, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.