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

Clearing the jsdom environment #1224

Closed
vvo opened this issue Jun 30, 2016 · 25 comments
Closed

Clearing the jsdom environment #1224

vvo opened this issue Jun 30, 2016 · 25 comments

Comments

@vvo
Copy link
Contributor

vvo commented Jun 30, 2016

Currently between two it() calls, any DOM node will stay, so you have to manually clean it up.

Should we:

  • provide a way to easily clean the jsdom env? (besides removeChild or innerHTML, is there any jest.clearDOM()?)
  • automatically clean it for the user between to it() (as an option)
  • do nothing, just clean your own mess

Also relevant, when I used mocha, I was using this very simple tool: https://github.com/rstacruz/jsdom-global

@vvo vvo changed the title Clearing the jsdom environement Clearing the jsdom environment Jun 30, 2016
@cpojer
Copy link
Member

cpojer commented Jul 1, 2016

Cleanup doesn't really do much special stuff: https://github.com/rstacruz/jsdom-global/blob/master/index.js#L49

I suggest if you need this, to either:

  • Write your own cleanup handler.
  • Create a separate test file.

Does this work for you? At this point I'm quite hesitant to adding new APIs to support jsdom better.

@vvo
Copy link
Contributor Author

vvo commented Jul 1, 2016

Wfm

@vvo vvo closed this as completed Jul 1, 2016
@cameron-martin
Copy link

It seems to me that it'd be best for jest-environment-jsdom to create a new instance of JSDOM for each test. This way less state is maintained between tests.

Is there any reason this isn't done currently?

@cpojer
Copy link
Member

cpojer commented Nov 30, 2018

We do create new instances of jsdom, however we do not load jsdom from scratch which would increase runtime for each test by more than 100ms.

@cameron-martin
Copy link

That's a shame that instantiating jsdom is so expensive. I'll open an issue on the jsdom repo for discussion on either:

  • Decreasing this startup time.
  • Providing a cheap way of resetting jsdom's state.

@granmoe
Copy link

granmoe commented Nov 30, 2018

Just chiming in to say that this would be an absolutely amazing feature to have for the already incredible library that is Jest.

On the other hand, perhaps we should petition for a solution over in the jsdom repo?

@granmoe
Copy link

granmoe commented Nov 30, 2018

(Assuming perf issues could somehow be alleviated, of course.)

@vvo
Copy link
Contributor Author

vvo commented Dec 3, 2018

For what it's worth, last time we needed that we used:

  beforeEach(() => {
    document.body.innerHTML = '';
  });

@cameron-martin
Copy link

The issue with doing this is that you have to know which state is modified during each test, which leaves a lot of room for human error.

@ryx
Copy link

ryx commented Dec 5, 2018

The problem with simply clearing the "physical" DOM (i.e. innerHTML) is also that the shared state for virtual objects (window.*) still remains. This can result in very subtle errors (e.g. a single test running fine on its own but failing when running in a suite). This seems to be a common problem when using Object.defineProperty on globals like localStorage, navigator.*, etc..

I'd really vote for a more convenient way in jest-environment-jsdom that allows to reset the entire JSDOM on demand (e.g. something like jsdom.reset()). That way it would be possible to explicitly accept the performance penalty and have a reliable way to clean up.

@cameron-martin
Copy link

A configuration option for jest-environment-jsdom would work.

@granmoe
Copy link

granmoe commented Dec 5, 2018

It occurs to me that there's a point at which integration tests are close enough to end-to-end tests that they should just be made into E2E. However, this doesn't apply to the situations in which we're seeing this JSDOM issue on our team. Plus, there are often good reasons to do certain things as integration tests via jest instead of E2E. Just in case anyone was wondering, which they most assuredly were not.

@SimenB
Copy link
Member

SimenB commented Dec 6, 2018

If you need a fresh environment for every single test, just put them in individual test files. The jsdom env is recreated for each test file. I don't think an API for this makes much sense, as it's quite edge casey

@m0rjc
Copy link

m0rjc commented Jan 23, 2019

I'm testing code based on Window.postMessage() at the moment, using event listeners to look for messages coming from the component under test. or instantiating it and having it listen.

I've managed to solve a fair amount by creating an iframe, using its contentWindow, then throwing that away. But I'm still using the global window object in some tests.

@kohlmannj
Copy link

We're writing some tests that inject <script> tags into the DOM, but only if a certain window global doesn't already exist. It'd help us to have some kind of reset method for the JSDOM window instance so that we don't have to worry about performing cleanup for each specific window global.

@BlackGlory
Copy link
Contributor

Without this feature, it seems impossible to reset properties like window.history.

@jhildenbiddle
Copy link

jhildenbiddle commented Jul 20, 2020

Slow and working is better than fast and broken.

Jest should offer the ability to reset jsdom on each test and let users determine if the extra 1/10th of a second performance hit per reset is tolerable for them. Examples have been provided here that clearly demonstrate why this is necessary and how simple resets like document.body.innerHTML = "" are insufficient.

Optimizing for performance is fine, but neglecting to offer a "slower but safer" path simply because it adds a minuscule amount of time per test is hostile to end users and feels weirdly (and unnecessarily) performance obsessed.

@jhildenbiddle
Copy link

jhildenbiddle commented Oct 25, 2020

For those interested, here is the soft-reset I'm using in jest.setup-tests.js which does the following:

  • Removes event listeners added to document and window during tests
  • Removes keys added to document and window object during tests
  • Remove attributes on <html> element
  • Removes all DOM elements
  • Resets document.documentElement HTML to <head></head><body></body>
const sideEffects = {
  document: {
    addEventListener: {
      fn: document.addEventListener,
      refs: [],
    },
    keys: Object.keys(document),
  },
  window: {
    addEventListener: {
      fn: window.addEventListener,
      refs: [],
    },
    keys: Object.keys(window),
  },
};

// Lifecycle Hooks
// -----------------------------------------------------------------------------
beforeAll(async () => {
  // Spy addEventListener
  ['document', 'window'].forEach(obj => {
    const fn = sideEffects[obj].addEventListener.fn;
    const refs = sideEffects[obj].addEventListener.refs;

    function addEventListenerSpy(type, listener, options) {
      // Store listener reference so it can be removed during reset
      refs.push({ type, listener, options });
      // Call original window.addEventListener
      fn(type, listener, options);
    }

    // Add to default key array to prevent removal during reset
    sideEffects[obj].keys.push('addEventListener');

    // Replace addEventListener with mock
    global[obj].addEventListener = addEventListenerSpy;
  });
});

// Reset JSDOM. This attempts to remove side effects from tests, however it does
// not reset all changes made to globals like the window and document
// objects. Tests requiring a full JSDOM reset should be stored in separate
// files, which is only way to do a complete JSDOM reset with Jest.
beforeEach(async () => {
  const rootElm = document.documentElement;

  // Remove attributes on root element
  [...rootElm.attributes].forEach(attr => rootElm.removeAttribute(attr.name));

  // Remove elements (faster than setting innerHTML)
  while (rootElm.firstChild) {
    rootElm.removeChild(rootElm.firstChild);
  }

  // Remove global listeners and keys
  ['document', 'window'].forEach(obj => {
    const refs = sideEffects[obj].addEventListener.refs;

    // Listeners
    while (refs.length) {
      const { type, listener, options } = refs.pop();
      global[obj].removeEventListener(type, listener, options);
    }

    // Keys
    Object.keys(global[obj])
      .filter(key => !sideEffects[obj].keys.includes(key))
      .forEach(key => {
        delete global[obj][key];
      });
  });

  // Restore base elements
  rootElm.innerHTML = '<head></head><body></body>';
});

This has worked well for me so far. Not as well as a native Jest reset likely would, but... you know. 😉

@trusktr
Copy link

trusktr commented Nov 1, 2020

Nice @jhildenbiddle.

Also, it is easy to import jsdom and just create your own context in your tests, regardless if you're even using Jest or not.

For example, you can make a function like this:

const { JSDOM } = require('jsdom');

function initJSDOM(markup, options = {}) {
  const dom = new JSDOM(markup, options);

  global.window = dom.window;
  global.document = dom.window.document;
  global.navigator = dom.window.navigator;
  global.location = dom.window.location;
  global.XMLHttpRequest = dom.window.XMLHttpRequest;

  return dom;
}

then use initDOM in any test to make a brand new JSDom context.

@elias6
Copy link

elias6 commented Nov 7, 2020

@jhildenbiddle overall your idea looks impressive. But I have a question. Is it OK to set the innerHTML property of document.documentElement to something with an html tag? I looked at document.documentElement.innerHTML on some random pages and it seems to only include the stuff inside the html tag, not the html tag itself.

The obvious solution would be to assign to document.documentElement.outerHTML instead, but that doesn't work when I try it. I get errors saying NoModificationAllowedError: Modifications are not allowed for this document. I can't think of anything better.

@jhildenbiddle
Copy link

jhildenbiddle commented Nov 7, 2020

Good catch, @elias6.

The following line:

  rootElm.innerHTML = '<html><head></head><body></body></html>';

Should be:

  rootElm.innerHTML = '<head></head><body></body>';

I've updated the code in the comment above.

FYI, here's the explanation for why you were unable to set document.documentElement.outerHTML from MDN:

If the element has no parent element, setting its outerHTML property will not change it or its descendants. Many browsers will also throw an exception.

Since document.documentElement is the <html> element, it has no parent node and therefore throws an error.

@elias6
Copy link

elias6 commented Nov 9, 2020

@jhildenbiddle what if there are attributes on the html element? How can those be cleaned up? I am doing something like this in my tests:

[...document.documentElement.attributes].forEach(attr =>
	document.documentElement.removeAttribute(attr.name)
);

But I am wondering if that catches everything, or if there is a somehow better way to do what I am doing.

@jhildenbiddle
Copy link

@elias6 --

I'm doing the same thing in the beforeEach block of the reset:

const rootElm = document.documentElement;

// Remove attributes on root element
[...rootElm.attributes].forEach(attr => rootElm.removeAttribute(attr.name));

No reason to think it wouldn't remove all of the attributes since that's exactly what removeAttribute() is supposed to do. :)

@elias6
Copy link

elias6 commented Nov 11, 2020

@jhildenbiddle I know that my idea removes all the attributes on the html element. I was asking about whether there is any other important state that it fails to remove. But I'm guessing the code in your comment handles that.

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests