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

It should be possible to load different fixture files in tests depending on a browser #2480

Closed
f1ames opened this issue Sep 13, 2018 · 5 comments · Fixed by ckeditor/ckeditor5-paste-from-office#16
Labels
package:paste-from-office type:task This issue reports a chore (non-production change) and other types of "todos".
Milestone

Comments

@f1ames
Copy link
Contributor

f1ames commented Sep 13, 2018

It is already mentioned in https://github.com/ckeditor/ckeditor5-paste-from-office/issues/1#issue-347950013 (2. Testing on different browsers). For many simple cases the raw clipboard data is the same when pasted from Word to Chrome, Firefox and Edge. However, it differs for Safari (and will also differ for more complex scenario like pasting images in other browsers).

At the moment fixtures are simply loaded via import:

import boldWithinText from '../../_data/basic-styles/bold-within-text/input.word2016.html';

It could be done manually in each test file like:

import boldWithinText from '../../_data/basic-styles/bold-within-text/input.word2016.html';
import boldWithinTextSafari from '../../_data/basic-styles/bold-within-text/input.safari.word2016.html';

but it produces a lot of redundant code. And it can be only done if input.safari.word2016.html file is present so all loading will need to be done manually.

So it should be possible to load fixtures like:

import { loadFixtures } from '../../_utils/utils';
const fixtures = loadFixtures( 'basic-styles' );
console.log( fixtures );
// { 
//     boldWithinText: 'html string...', 
//     underlinedText: 'html string...'
// }

Internally the loadFixtures will:

  • Check the current browser.
  • Try to load input.BROWSER.word2016.html file.
  • If there is no such file it will load the default input.word2016.html.

It will be also useful if the function can find all fixtures placed in a given folder/group itself (here basic-styles) so all the fixtures names don't have to be passed as an input to this function.

@f1ames
Copy link
Contributor Author

f1ames commented Sep 14, 2018

@pomek (as a person who contributed to our test tools the most) could you take a look on the above and tell if there are any ready mechanisms which we can utilize to achieve this. Or maybe we already tried to solve similar issues in the past? What do you think will be the best way to tackle this task?

@pomek
Copy link
Member

pomek commented Sep 14, 2018

What do you about a simple module that could export a function that will return required textures fixtures?

import basic_styles_safari from '...';
import basic_styles_default from '...';

export default function getFixtures( package ) { 
    // Check current browser.
    // Check whether textures exists.
    // etc.
    
    return /* fixtures */ 
};

Something similar to OperationFactory. I know that you must remember about adding new fixtures to the module manually but dynamic imports won't work without additional works in Karma configuration.

@Reinmar
Copy link
Member

Reinmar commented Sep 14, 2018

What about webpack's dynamic imports?

@f1ames
Copy link
Contributor Author

f1ames commented Sep 14, 2018

When it comes to dynamic imports, it could be done like:

import( `.../${ browser }.fixturename.html` )
    .then( data => { fixtures[ name ] = data; } )
    .catch( err => {
        return import( '.../default.fixturename.html' );
    } )
    .then( data => { fixtures[ name ] = data } );

But then you still need a list of fixture names (just default ones from which browser specific names could be composed).

And there is a problem with our ESLint configuration as it:

  • Allows import to be only a top level statement so you cannot wrap it inside function, for, etc.
  • Does not allow syntax like import( 😭

And one more thing, when dynamically imported file does not exist, there is an error logged in a console during test run:
image
To be more precise it contains "Error" message but it is logged as an Info not an Error.

@f1ames
Copy link
Contributor Author

f1ames commented Sep 17, 2018

I have created a minimalistic version with static fixtures loading to see how (un)pleasant using it can be. It is doable but requires excessive amount of typing.

The fixture loader is quite simple. It imports all fixtures indexes and generates an object with fixtures based on a browser in which tests are run (see https://github.com/ckeditor/ckeditor5-paste-from-office/blob/c2e7b96e1ddec587656d1515d3b6904238505f13/tests/_utils/fixtures.js).

A fixture index is a file like tests/_data/fixtures-group/index.js which imports all the HTML files for a given group and exports them as an object so fixture loader can use them. I have extracted it, because placing all in the one file will simply make it unmaintainable. Example index file for basic-data fixtures - https://github.com/ckeditor/ckeditor5-paste-from-office/blob/c2e7b96e1ddec587656d1515d3b6904238505f13/tests/_data/basic-styles/index.js. So creating an index file is a pain-point here, but it have to be done only once so I guess we can live with it for now. WDYT?


I dug a little bit deeper to check if it would be feasible to adjust our ESLint config to enable dynamic imports. However, it seems that ESLint doesn't support nested import and import() syntax at all - eslint/eslint#7764 (comment) and https://stackoverflow.com/a/39173590/646871 (babel-eslint is recommended in such cases).

Reinmar referenced this issue in ckeditor/ckeditor5-paste-from-office Nov 5, 2018
Other: Support for pasting flat list and spacing handling for pasted content in Safari. Closes #1. Closes #12. Closes #15.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-paste-from-office Oct 9, 2019
@mlewand mlewand added this to the iteration 21 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:task This issue reports a chore (non-production change) and other types of "todos". package:paste-from-office labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:paste-from-office type:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants