Skip to content

dependencies are duplicated for each test file #49

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

Open
LarsDenBakker opened this issue Mar 9, 2019 · 19 comments
Open

dependencies are duplicated for each test file #49

LarsDenBakker opened this issue Mar 9, 2019 · 19 comments

Comments

@LarsDenBakker
Copy link

LarsDenBakker commented Mar 9, 2019

Each test file is treated as a separate bundle. Common dependencies are duplicated between the test files.

This is wasteful, and creates extra overhead for large project. But more importantly it creates issues when working with web components, as they may only be registered once per document. Bundling the same web component twice will throw an error.

I creates a reproduction for this: https://github.com/LarsDenBakker/karma-rollup-preprocessor-issues

npm install
npm run test:dynamic-import
optionally, debug the code at: http://localhost:9876/debug.html

You will see the error being thrown. If you inspect the output bundles in the browser, you will see the web component code bundled twice.

We are switching from webpack to rollup for our recommended setup for developers working with web component at: https://open-wc.org/

We had a similar issue with webpack codymikol/karma-webpack#379 which we were able to solve.

Happy to help out where possible.

@AgDude
Copy link

AgDude commented Jun 7, 2019

This issue also makes debugging tests in a browser very difficult. Since the source is duplicated, sourcemaps are unreliable, making it is difficult to put breakpoints in the right place.

@AgDude
Copy link

AgDude commented Jun 7, 2019

I am able to mostly avoid this problem by creating a single entry point for tests.

Create an entry point like: find src -name '*.spec.js' | awk '{print "export * from '\''./" $0 "'\'';"}' > karma-entry.spec.js

Then instead of including /src/**/*.js include only karma-entry.spec.js

It would be nice if the preprocessor could handle this automatically, but incorporating this in our npm script seems to solve the problem for us.

@tommck
Copy link

tommck commented Feb 6, 2020

Wow.. I wish I had realized this before I converted by old webpack build to rollup. This also makes testing incredibly slow! It's honestly unusable in its current state :(

@Lalem001
Copy link
Contributor

Lalem001 commented Mar 5, 2020

@tommck The problem is in Karma, not Rollup (or karma-rollup-preprocessor). If you see Karma Webpack's Alternative Usage section, it too suggests a single entry point referencing all other files for more performance.

@tommck
Copy link

tommck commented Mar 5, 2020

@Lalem001 and ... rollup doesn't support importing a bunch of test files all into one in a simple way like webpack does. So... rollup doesn't have any built-in way to mitigate this. I'd say that's still a rollup + karma problem, since it's solved w/ webpack

@Lalem001
Copy link
Contributor

Lalem001 commented Mar 16, 2020

@tommck

rollup doesn't support importing a bunch of test files all into one

Officially, it does. See @rollup/plugin-multi-entry

I'd say that's still a rollup + karma problem, since it's solved w/ webpack

Webpack + Karma has the same problems as rollup + karma. The link I posted in my previous comment talks about this. Karma forces Webpack/Rollup to generate a bundle for EACH test. The way around it, the "alternative usage", is to have a single entry file that imports all other tests.

@tommck
Copy link

tommck commented Mar 16, 2020 via email

@tommck
Copy link

tommck commented Mar 25, 2020

@Lalem001 multiple entry points is exactly the OPPOSITE if what we need here. We need 1 entry point that pulls in all spec files so that it's ONE bundle

@Lalem001
Copy link
Contributor

@tommck I know, and that is what I suggested above. A single entry point that imports all others.

@tommck
Copy link

tommck commented Apr 2, 2020

...which we have to create manually

@Lalem001
Copy link
Contributor

Lalem001 commented Apr 2, 2020

@tommck Not necessarily. Like webpack (webpack-import-glob-loader), Rollup can be configured to import with glob (rollup-plugin-glob-import).

In which case you could have your entry file look like the following:

import '**/*.spec.js';

Please pay attention to the fact that Webpack too must be configured to work this way, which is what I have been saying all along. Webpack faces the same issues as Rollup when it comes to working within the confines of Karma. Fixing this issue requires changes to Karma itself.

@jlmakes
Copy link
Owner

jlmakes commented Apr 2, 2020

I'd like to drop support for Rollup@1 and Node@8 and release 8.0.0, which will be a great opportunity to update this library's documentation to help with this situation. Any suggestions?

@Lalem001
Copy link
Contributor

Lalem001 commented Apr 3, 2020

Honestly, just a section similar to the one in Karma Webpack's readme would suffice. Single entry does come with caveats in my experience though. Karma watcher doesn't really work since it is only working with a single file. And the glob import scripts linked above do not see high amounts of usage.

@Danielku15
Copy link

@Lalem001 Thanks for the hint with the globbing. Luckily I was already in the situation of having an own "resolver" plugin in my project so I could extend it for my needs. Maybe it helps somebody, here the rough steps that I did:

Step 1: Add a new index test file importing all tests/specs with globbing.

test/index.js
import '**/*.test.js';

Step 2: Add a custom rollup plugin to your project.

rollup.test-glob.js
const join = require('path').join;
const glob = require('glob').sync;

module.exports = function (mappings) {
    return {
        name: 'test-glob',
        resolveId: function (importee, importer) {
            // required to mark glob file pattern as resolved id
            if (importee && importee.startsWith('**')) {
                return importee;
            }
            return null;
        },
        load(id) {
            if (id.startsWith('**')) {
                // glob files and generate source code like
                // import _0 from 'C:\dev\test\file01.test.js'; export { _0 };
                // import _1 from 'C:\dev\test\file02.test.js'; export { _1 };
                // ...
                const files = glob(id, {
                    cwd: process.cwd()
                });
                const source = files
                    .map((file, i) => `import _${i} from ${JSON.stringify(join(process.cwd(), file))}; export { _${i} };`)
                    .join('\n');
                return source;
            }
            return null;
        }
    };
};

Step 3: Use the plugin in your karma.conf.js.

karma.conf.js
const testGlob = require("./rollup.test-glob");

module.exports = function (config) {
  config.set({
    files: [
      { pattern: "test/index.js", watched: false },
    ],
    preprocessors: {
      "test/index.js": ["rollup"],
    },
    rollupPreprocessor: {
      plugins: [
        testGlob()
      ],
    },
  });
};

Of course you can also directly embed the "plugin" in your normal karma.conf and use it.

The solution is not perfect and a built-in solution would be nicer but it works smooth for me. 1 test bundle, quick compilation.

@jasonwaters
Copy link

I am able to mostly avoid this problem by creating a single entry point for tests.

Create an entry point like: find src -name '*.spec.js' | awk '{print "export * from '\''./" $0 "'\'';"}' > karma-entry.spec.js

Then instead of including /src/**/*.js include only karma-entry.spec.js

It would be nice if the preprocessor could handle this automatically, but incorporating this in our npm script seems to solve the problem for us.

I did this, but now my coverage report doesn't have the same breakouts as it used to. any idea how to get the separate reports again?

@jlmakes
Copy link
Owner

jlmakes commented Apr 25, 2020

@jasonwaters The single entry point makes sense for speeding up development, but I would use a different Karma config to generate coverage (using an environment variable to do so only in CI).

@Munter
Copy link

Munter commented May 18, 2020

We've tried using this plugin in Mochajs as well, and have decided against it becasue of the massive slowdown we experience. I have attempted to write our own karma plugin that takes all matching file globs and bundles them into a single bundle. This is done by mapping the bundle globs to a bundle path and replacing the globs in karmas file list with the bundle path and add a preprocessor for that specific file. The preprocessor then takes all the files associated with that bundle in the framework callback and bundles all of them.

This has been built only for mocha's purposes, and is so fresh off the press that it hasn't been tested properly yet. But maybe it can serve as an inspiration for solving the performance problem for this plugin.

https://github.com/mochajs/mocha/pull/4293/files#diff-fcd69381b04b5bc3dc3fa222e39fff44

@Aaronius
Copy link

FWIW, we updated a project to use rollup-plugin-glob-import like @Lalem001 mentioned and it sped up our test runs from about 60 seconds to 13 seconds (from hitting enter to process complete). Our coverage report is still broken out by source file as well. Feel free to take a look at the changes that were made here: https://github.com/adobe/alloy/pull/510/files

@jlmakes
Copy link
Owner

jlmakes commented May 29, 2020

Single entry does come with caveats in my experience though. Karma watcher doesn't really work since it is only working with a single file.

@Lalem001 I've already implemented a custom file watcher to trigger re-bundling when module dependencies change, and it seems feasible to generate a file at runtime that imports all of the karma preprocessor entry points.

Do you have any thoughts on adding rollup-plugin-glob-import as a preprocessor dependency and implementing something under the hood similar to what @Aaronius shared?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants