T/16938 Extract common PFW tests loop. #333
Conversation
| testData = options.testData || { _should: { ignore: {} } }, | ||
| ignoreAll = options.ignoreAll === true, | ||
| compareRawData = options.compareRawData === true, | ||
| customFilters = options.customFilters ? options.customFilters : null; |
There was a problem hiding this comment.
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 ); |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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++ ) { |
There was a problem hiding this comment.
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 ); |
There was a problem hiding this comment.
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 ); |
There was a problem hiding this comment.
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 | |||
There was a problem hiding this comment.
If we're adding docs in other places, then we can also add missing docs for createTestCase as only one parameter is described.
Proposed merge commit message:
Tests: Common Paste from Word test loop extracted as a helper.