search

ckeditor / ckeditor4

The best enterprise-grade WYSIWYG editor. Fully customizable with countless features and plugins.
https://ckeditor.com/ckeditor-4
Other
5.79k stars 2.48k forks source link

Refactor PfW testing infrastructure #3265

Open Comandeer opened 5 years ago

Comandeer commented 5 years ago

Type of report

Task

Provide description of the task

Issues

As #3258 introduces the whole new model of pasting, it would be nice to have generic system for testing pasting different types of content. We already have such system inside CKE4 and it's dedicated to testing PfW. However it's far from ideal:

  1. it uses Word nomenclature;
  2. it's divided into several files, that must be loaded before tests, resulting in tons of bender include comments;
  3. it has really complicated configuration;
  4. it uses CKE4 ajax plugin to load fixtures;
  5. it tries to guess fixtures paths, which results in many, really many, HTTP requests ending in 404 error; it also leads to bugs (#3148).

Proposed solutions

I propose two solutions for mentioned issues that can be implemented separately or jointly:

New configuration API

Current issues

ATM we do not list fixtures per se, but list browsers and word versions in which we created fixtures. E.g.:

bender.test( createTestSuite( {
    browsers: [
        'chrome',
        'firefox',
        'ie8',
        'safari'
    ],
    wordVersions: [
        'office365',
        'word2013'
    ],
    tests: {
        'Page_break_simple': true
    }
} ) );

generates following HTTP requests (HTTP status after hyphen):

21 requests, 11 HTTP 404 errors (more than 50%!) and 7 non-cacheable (due to query string) requests to the same expected.html file.

Proposed solution

Instead we can think of basing our loading system on treating expected.html as a key in fixtures map and operate on paths:

bender.test( createTestSuite( {
    tests: {
        'Page_break_simple/expected.html': [
            'Page_break_simple/office365/chrome.html',
            'Page_break_simple/office365/firefox.html',
            'Page_break_simple/office365/safari.html',
            'Page_break_simple/word2013/expected_ie8.html'
        ],
        'Page_break_simple/word2013/expected_ie8.html': [
            'Page_break_simple/word2013/ie8.html'
        ]
    }
} ) );

This way we have the visible relation between which inputs produce which output. We also list all fixtures, so no more 404 HTTP requests (if they occur, they should be treated as failing tests). We can slightly enhance this format to add also info about which browsers should perform which tests:

bender.test( createTestSuite( {
    tests: {
        'Page_break_simple/expected.html': {
            inputs: [
                'Page_break_simple/office365/chrome.html',
                'Page_break_simple/office365/firefox.html',
                'Page_break_simple/office365/safari.html',
                'Page_break_simple/word2013/expected_ie8.html'
            ],

            browsers: !CKEDITOR.env.ie || CKEDITOR.env.version > 8
        },
        'Page_break_simple/word2013/expected_ie8.html': {
            inputs: [
                'Page_break_simple/word2013/ie8.html'
            ],

            browsers: CKEDITOR.env.ie && CKEDITOR.env.version === 8
        }
    }
} ) );

IMO still much readable than our current format and containing all needed info. If some other options are needed, they can be added as browsers one for particular cases:

bender.test( createTestSuite( {
    tests: {
        'Page_break_simple/expected.html': {
            inputs: [
                'Page_break_simple/office365/chrome.html',
                'Page_break_simple/office365/firefox.html',
                'Page_break_simple/office365/safari.html',
                'Page_break_simple/word2013/expected_ie8.html'
            ],

            browsers: !CKEDITOR.env.ie || CKEDITOR.env.version > 8,
            compareRawData: true,
            customFilters: [ pfwTools.filters.font ]
        }
    }
} ) );

In case of tests for images, which require also RTF fixtures, we can think of something like:

bender.test( createTestSuite( {
    tests: {
        'Page_break_simple/expected.html': {
            inputs: [
                {
                    'text/html': 'Page_break_simple/office365/chrome.html',
                    'text/rtf': 'Page_break_simple/office365/chrome.rtf'
                },

                {
                    'text/html': 'Page_break_simple/office365/firefox.html',
                    'text/rtf': 'Page_break_simple/office365/firefox.rtf'
                }
            ]
        }
    }
} ) );

This syntax would also allow us to test even exotic MIME types in case we add some other paste handlers in the future. IMO we can handle two syntaxes for inputs parameter:

Or we can stick only to the latter syntax (still readable, but far more flexible).

Summary

So we'd end with something like that:

bender.test( createTestSuite( {
    tests: {
        'Page_break_simple/expected.html': {
            inputs: [
                {
                    'text/html': 'Page_break_simple/office365/chrome.html',
                    'text/rtf': 'Page_break_simple/office365/chrome.rtf'
                },

                {
                    'text/html': 'Page_break_simple/office365/firefox.html',
                    'text/rtf': 'Page_break_simple/office365/firefox.rtf'
                }
            ],

            browsers: !CKEDITOR.env.ie || CKEDITOR.env.version > 8,
            compareRawData: true,
            customFilters: [ pfwTools.filters.font ]
        }
    }
} ) );

Such API should cover all (or at least most) cases we have in PfW nowadays and should be easy to extend in the future (due to the ability to handle any MIME type out of the box). It is also free of Word-specific nomenclature and purely based on paths to the fixtures.

Generator of test files

Changing API to the new format would allow easier automatic generation of test files. The generator would just have to fetch all files from given directory and output them as an array. Automatic generation will prevent errors with wrong file names and will speed the creation of new tests cases.

We can include such tool as one of our dev/ tools inside CKE4 dev repo and it will ensure that our PfW tests are integral and uses all fixtures correctly.

WDYT?

jacekbogdanski commented 5 years ago

I missed somehow in your description point about additional bender plugin (is it still required?), but overall it looks nice. Reversing Input/Output fixtures should give more flexibility when preparing fixtures. Especially, that proposed API allows choosing a browser for a single fixture (AFAIK currently it works only for the whole test suite).

Also, the input object could be extended in the future with additional options. Maybe it makes sense to create an additional namespace for input types to avoid issues with future enhancements? Something like:

inputs: [ {
  customFilters: [ pfwTools.filters.font ],
  dataTransfer: {
    'text/html': 'path/to/html',
    'text/rtf': 'path/to/rtf'
  }
} ]

In the above use case, you will get better control over additional test case configuration when e.g. filters should be only appended into this single test. However, it will complicate a bit API, so maybe look for dataTransfer if available, otherwise, treat the whole object as dataTransfer getter?

On the other hand, you can still get the same result by extracting test input into the separate test case, so not sure if it's worth complicating things.

Comandeer commented 5 years ago

I missed somehow in your description point about additional bender plugin (is it still required?), but overall it looks nice.

The point is to get rid of unreadable, manual imports:

/* bender-include: ../../pastefromword/generated/_helpers/promisePasteEvent.js,../../pastefromword/generated/_helpers/assertWordFilter.js,../../pastefromword/generated/_helpers/createTestCase.js */
/* bender-include: ../../pastefromword/generated/_helpers/createTestSuite.js,../../pastefromword/generated/_helpers/pfwTools.js */
/* global createTestSuite */

Enclosing test infrastructure in plugin allows us to use it in every plugin, without the need to think about what we have to import. Especially when we use only createTestSuite from all the imports.

Maybe it makes sense to create an additional namespace for input types to avoid issues with future enhancements?

I'm not sure if we should treat every input as separate test. I was rather thinking of treating whole expected – inputs pair as a single test. In such scenario we don't need namespace for inputs.

jacekbogdanski commented 5 years ago

Thanks for clarifying that.

I'm not sure if we should treat every input as separate test. I was rather thinking of treating whole expected – inputs pair as a single test. In such scenario we don't need namespace for inputs.

Yeah, taking into account that now it will be much easier to create separate test even for a single input, there is probably no need for additional IO configuration (at least for now).

f1ames commented 5 years ago

I wonder how much getting rid of 404 can speed up tests execution 🤔

The general idea looks quite good, this refactoring can bring some benefits for sure 👍 The only concern is that in some tests we have a lot of fixtures used so files in a new format may become a little bulky:

tests: {
    'Bold': true,
    'Colors': true,
    'Custom_list_markers': true,
    'Fonts': true,
    'Image': true,
    'Italic': true,
    'Link': true,
    'Object': [ 'word2013' ],
    'Only_paragraphs': true,
    'Ordered_list': true,
    'Ordered_list_multiple': true,
    'Ordered_list_multiple_edgy': true,
    'Paragraphs_with_headers': true,
    'Simple_table': true,
    'Spacing': true,
    'Text_alignment': true,
    'Underline': true,
    'Unordered_list': true,
    'Unordered_list_special_char_bullet': true,
    'Table_alignment': true,
    'Table_vertical_alignment': true
},

but still should be readable I suppose.

And then we have ignoring of specific test cases:

testData: {
    _should: {
        ignore: {
            'test Object word2013 datatransfer': CKEDITOR.env.edge,
            'test Unordered_list_special_char_bullet word2013 chrome': CKEDITOR.env.edge,
            'test Unordered_list_special_char_bullet word2013 firefox': CKEDITOR.env.edge
        }
    }
},

which also needs to be handled (maybe similar way that it is now)?

Apart from that it's 👍 from me. We can try to tackle this issue as a follow-up after closing #835.

@Comandeer we could start with adjusting the architecture in CKEditor 4 tests itself and then eventually go with bender plugin if we still find it necessary. WDYT?

Comandeer commented 5 years ago

And then we have ignoring of specific test cases: […] which also needs to be handled (maybe similar way that it is now)?

Won't it be covered by browsers field in the proposed syntax? The only downside is that it would appear in every test instead of one central place. But is it really a downside? 🤔

we could start with adjusting the architecture in CKEditor 4 tests itself and then eventually go with bender plugin if we still find it necessary. WDYT?

Sounds sensible 👍