Prevent logging all the JSDOM internal errors.

[Aftabnack]

Summary

• Fixes Hide JSDOM "not implemented" errors #5266

Test plan

Not sure what to do for this.

[SimenB]

@kentcdodds thoughts on this?

[Aftabnack]

The failing tests that are shown here, ran locally without failing. I'm on 8.6.0. Anything I am missing?

[kentcdodds]

I decided to not use omitJSDOMErrors for two reasons:

1. The giant log I was seeing which resulted in pass testConsole to TestEnvironment #5227 also appears in the browser so it makes sense we see it in the JSDOM environment as well.
2. @domenic said: "jsdomErrors are fired for many other cases as well, besides JavaScript errors. E.g. failed CSS parsing or image loading."

Because #5227, the console JSDOM uses is the same one you have in your tests, so it's mockable now. So if you don't like the errors that are logged, you can simply do:

beforeEach(() => {
  jest.spyOn(console, 'error')
  console.error.mockImplementation(() => {})
})

afterEach(() => {
  console.error.mockRestore()
})

So I would recommend against making this change.

[cpojer]

Thanks Kent. I agree. @Aftabnack thanks for sending a pull request and I hope you'll understand that we do not want to merge it at this time. If you feel strongly, you can create your own test environment with this configuration as a default.

[Aftabnack]

• If anyone is coming here with the same issue, I have made a separate package that you can use as a drop-in replacement for jsdom: jest-env-jsdom-silent. Read the thread below

[kentcdodds]

FWIW, the error stack trace in the screenshot on that README is indicative of an actual problem. It looks like Artifacts.doDownload in description-view/Artifacts.phnx is expecting to be able to use navigation but because that's not implemented in jsdom, jsdom is logging the error. You'll need to provide your own implementation/polyfill/mock of navigation or change description-view/Artifacts.phnx to not use it. Otherwise your test could misbehave and cause you to miss a real bug.

This is why I recommend against making this change. Otherwise you will miss these valuable indicative errors.

[Aftabnack]

@kentcdodds Appreciate you taking time out for this.

• doDownload has just one liner which is setting window.location.href.
• Now this functionality actually works on all the browsers, but my testcases throws that error since it runs in jsdom where it is not implemented yet.
• So I find it right to hide those errors since updating window.location.href is guaranteed to work, and I shouldn't test it since browser implementations would have already tested it.

[kentcdodds]

Yeah. What I would do is provide my own implemention/switch it to use location.assign and mock that 👍

[Aftabnack]

That actually makes more sense. Something like this?

import changeHref from '../common';

doDownload() {
    changeHref(myUrl);
}

• And while writing testcases, mock changeHref to be a no-op?

[kentcdodds]

No, I mean in your source code use location.assign instead of assigning to location.herf, then spyOn location.assign with a mock implemention (similar to my console.error mock above).

[Aftabnack]

@kentcdodds I was going over this again while writing testcases, I began to wonder.

My code that I have written is expected to run on latest browsers (Firefox/Chrome) which have these sort of functionality. So I shouldn't be changing my code just to make it run on jest.

Thoughts?

[kentcdodds]

I agree with you philosophically, but the fact of the matter is that those APIs are not supported by jsdom. You could try to implement them yourself if you like. I do that with localStorage and sessionStorage. The reason jsdom doesn't implement all APIs is because some of them are not possible to implement with 100% spec compliance. But most of the time you don't need to worry about edge cases so providing your own implementation isn't a big deal (and often isn't terribly difficult).

Good luck!

[Aftabnack]

Where is jest-env-chrome when we want it!! :P

PS: I've mocked localStorage and sessionStorage too.

[kentcdodds]

There is a jest-env-webdriver 😉

[Aftabnack]

Cool I'll try it out!!

[chrisbateman]

If you don't want to change your code to use location.assign - this seems to work with JSDom 11 and 13 (though there's a chance JSDom might break it in the future...)

delete window.location;
window.location = {}; // or stub/spy etc.

[kentcdodds]

I'm pretty sure that JSDOM 12 added support for localStorage and sessionStorage. I've removed my polyfills.

[github-actions]

This pull request 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.