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

EES-4715 suppress jsdom css errors #4428

Merged
merged 1 commit into from
Nov 27, 2023
Merged

EES-4715 suppress jsdom css errors #4428

merged 1 commit into from
Nov 27, 2023

Conversation

amyb-hiveit
Copy link
Collaborator

@amyb-hiveit amyb-hiveit commented Nov 23, 2023

JSDOM doesn't handle CSS well and seems to be particularly objecting to CKEditor, which is causing a ridiculous amount of errors in the log when running the admin tests. As far as I can tell it shouldn't be trying to parse it anyway as all CSS files should be going through the transform we have configured in jest.config.js, but it is so I've just supressed this error.

As suggested by @ntsim below, I've stubbed out CKEditor to prevent errors from its CSS build polluting the test output.

Comment on lines 13 to 20

const originalConsoleError = console.error;
const jsDomCssError = 'Error: Could not parse CSS stylesheet';
console.error = (...params) => {
if (!params.find(p => p.toString().includes(jsDomCssError))) {
originalConsoleError(...params);
}
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looked into the stylesheet a bit more and it seems like there's a few syntax errors which suggests something's potentially wrong with the built CSS. Most of the build config around this is from @ckeditor/ckeditor5-dev-utils, so maybe that's not quite configured correctly, or they may even have errors in their source CSS.

Unfortunately, the browser will just ignore stylesheet errors, so I guess this is slipping under the radar right now (one point for JSDOM). Hopefully they'll fix it in a later version of CKEditor.

Anyway, regardless of this issue, I think we can do a little better than just suppress the error. I figured out two options below:

Option 1 - Swap in VirtualTestEditor

This is a more complicated approach and is based on this issue, using Jest's moduleNameMapper we can stub out CKEditor (which doesn't really work in Node) with a VirtualTestEditor implementation. This can be found in ckeditor5-core, but isn't published as part of the package (check out source file).

Testing this out, I created a test build of explore-education-statistics-ckeditor which exports a copy-paste of the VirtualTestEditor:

// explore-education-statistics-ckeditor/src/ckeditor-test.js

import { DataApiMixin, Editor } from '@ckeditor/ckeditor5-core';

export default class VirtualTestEditor extends DataApiMixin(Editor) {
  constructor(config) {
    super(config);

    // Create the ("main") root element of the model tree.
    this.model.document.createRoot();
  }

  static create(config = {}) {
    return new Promise(resolve => {
      const editor = new this(config);

      resolve(
        editor
          .initPlugins()
          .then(() => editor.data.init(config.initialData || ''))
          .then(() => {
            editor.fire('ready');
            return editor;
          }),
      );
    });
  }
}

This just needs a few tweaks to the Webpack config:

// explore-education-statistics-ckeditor/webpack.config.js

+  entry: {
+    ckeditor: path.resolve(__dirname, 'src', 'ckeditor.js'),
+    'ckeditor-test': path.resolve(__dirname, 'src', 'ckeditor-test.js'),
+  },

   output: {
     // The name under which the editor will be exported.
     library: 'ClassicEditor',
     path: path.resolve(__dirname, 'build'),
+    filename: '[name].js',
     libraryTarget: 'umd',
     libraryExport: 'default',
   },

Once rebuilt, there's a build/ckeditor-test.js we can use to stub out the implementation. We can just swap this in through a moduleNameMapper in admin:

moduleNameMapper: {
  // ...
  '^explore-education-statistics-ckeditor$': '<rootDir>/../explore-education-statistics-ckeditor/build/ckeditor-test.js',
}

Option 2 - Stub out CKEditor

Iterating on option 1, we can actually go further and just do something like this:

// explore-education-statistics-admin/test/EditorStub.js

export default class EditorStub {}

Then just configure the moduleNameMapper appropriately:

moduleNameMapper: {
  // ...
  '^explore-education-statistics-ckeditor$': '<rootDir>/test/EditorStub.js',
}

This is a simpler approach and has some advantages:

  • Practically no code needs to be built (option 1 outputs ~300 KB of JS)
  • Basically nothing needs to be parsed during test runs

Giving it a light test, I didn't really see much of a difference in test performance on my machine. However, the above advantages might be more noticeable on lower end machines.

The main downside to this approach is that if in the future, we want to use a 'real' implementation of CKEditor in the tests, the stub obviously won't cut it.

The VirtualTestEditor implementation actually runs CKEditor and would allow actual methods to be called on it (etc). The main difference is that it doesn't try and render anything to the DOM (hence works great in Node).

For the timebeing, this isn't really necessary as we just swap out the CKEditor component (in FormEditor) with a textarea.

Personally, I'd go with option 2 and we can swap in option 1 if needed in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @ntsim, I've changed it to use option 2 now.

@amyb-hiveit amyb-hiveit merged commit 45ca60d into dev Nov 27, 2023
6 checks passed
@amyb-hiveit amyb-hiveit deleted the EES-4715 branch November 27, 2023 10:25
sergei-maertens added a commit to open-formulieren/formio-builder that referenced this pull request Dec 28, 2023
The webpack style injection in combination with JSDom does not
play nice together, so stub out the editor entirely.

Fix taken from dfe-analytical-services/explore-education-statistics#4428,
many thanks to the original author <3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants