SharedStylesHost isn't cleaning up styles between Karma test specs

[zelliott]

🐞 Bug Report

Affected package

Latest

Is this a regression?

No

Description

Wheen testing components via TestBed.createComponent, Angular appends inline styles associated with the component via the SharedStylesHost service. These inline styles are cleaned up when the service is destroyed.

Unfortunately, these inline styles are never cleaned up in Karma unit tests. This appears to be because each SharedStylesHost service is never destroyed, apparently because each associated NgModule is never destroyed between specs. This isn't a problem when you have <10 Karma specs, but when you have >100 , and each spec appends 25 <style> tags, by the 100th spec you have 2500 redundant <style> tags in your document head.

🔬 Minimal Reproduction

https://stackblitz.com/edit/angular-testing-exxbrh (old version of angular, but repros on latest)

🌍 Your Environment

Angular Version:

Floating version of Angular 2 RC branch. Repros on latest.

[zelliott]

Bump on this issue.

[mickaeltr]

Hello,

As a workaround, adding this to my jest.setup.ts made my whole test suite 2-3 times faster:

// Fixing this bug: https://stackoverflow.com/questions/51752862/angular-unit-tests-are-leaking-styles
afterEach(() => {
  window.document.querySelectorAll("style").forEach((style: HTMLStyleElement) => style.remove());
});

I use Jest but there should be a similar mechanism for Jasmine.

Hope this helps

[zelliott]

Another bump on this issue. This seems like a relatively straightforward fix with nontrivial performance gains.

[ersimont]

For the same workaround zelliott just mentioned, I made a function you can use called trimLeftoverStyles(), that you can get from my open source library s-ng-dev-utils. It will only remove the styles added by your components - if you added global styles that are used in your test suite it will leave them alone. If you call it in a beforeEach as recommended, it will also leave your component correctly styled at the end of your test, so you can see it correctly for debugging.

It would be great if this could be addressed in the framework itself!

[totszwai]

I believe this also happens in production if shared component from two different angular apps are running at the same time? What is the correct way to clean up all the added styles when the component is removed?

[zelliott]

Looks like this is being fixed in #38336.

[cexbrayat]

Until the fix lands, you can add to test.ts:

afterEach(() => {
  getTestBed().inject(ɵDomSharedStylesHost).ngOnDestroy();
});

Unlike the workaround mentioned above #31834 (comment), this only cleans the necessary stylesheets and not the global ones.

This speeds up our ng test task by 2-3x (from 120s to 40s on my machine) without breaking anything.

[mlicciardi]

@cexbrayat I've tried your suggestion, but I think I've messed up since no performance change

Until the fix lands, you can add to test.ts:

afterEach(() => {
  getTestBed().inject(ɵDomSharedStylesHost).ngOnDestroy();
});

Here's my test.ts file, can I have a review?

// This file is required by karma.conf.js and loads recursively all the .spec and framework files
import 'zone.js/dist/zone-testing';
import { getTestBed } from '@angular/core/testing';
import { ɵDomSharedStylesHost } from '@angular/platform-browser';
import {
  BrowserDynamicTestingModule,
  platformBrowserDynamicTesting,
} from '@angular/platform-browser-dynamic/testing';
import 'zone.js/dist/zone.js';
import 'zone.js/dist/async-test';
import 'zone.js/dist/fake-async-test';
import 'zone.js/dist/long-stack-trace-zone';
import 'zone.js/dist/proxy.js';
import 'zone.js/dist/sync-test';
import 'zone.js/dist/jasmine-patch';

declare const require: {
  context(
    path: string,
    deep?: boolean,
    filter?: RegExp
  ): {
    keys(): string[];
    <T>(id: string): T;
  };
};

// First, initialize the Angular testing environment.
getTestBed().initTestEnvironment(BrowserDynamicTestingModule, platformBrowserDynamicTesting());
// Patch to ensure SharedStylesHost cleanup styles between Karma test specs
afterEach(() => getTestBed().inject(ɵDomSharedStylesHost).ngOnDestroy());
// Then we find all the tests.
const context = require.context('./', true, /\.spec\.ts$/);
// And load the modules.
context.keys().map(context);

Quite sure that's not how it was supposed to be added ...

[elesueur]

@AndrewKushnir what's blocking the MR in 38336 and resolution of this issue?

This issue is pretty significant and for large apps could have a MAJOR impact on unit test execution. An issue like this opened in July 2019 and still not resolved boggles my mind.

[AndrewKushnir]

@elesueur the fix for the described issue would be a breaking change and we are looking at possible ways on how to make it less break-y for users.

[reduckted]

Am I correct in saying this has been fixed in Angular 13?

https://blog.angular.io/angular-v13-is-now-available-cce66f7bc296#1cd7

It seems that the default behavior is to remove styles. The old behavior can be kept using:

getTestBed().initTestEnvironment(BrowserDynamicTestingModule, platformBrowserDynamicTesting(), {
    teardown: { destroyAfterEach: false }
});

[cexbrayat]

@reduckted Yes exactly, this issue is fixed on v13 by default (94ba59b)

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}