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

Cleanup globals after tests #11

Closed
geowarin opened this issue Sep 18, 2015 · 9 comments
Closed

Cleanup globals after tests #11

geowarin opened this issue Sep 18, 2015 · 9 comments

Comments

@geowarin
Copy link

Hi there!

Thank you so much for the lib, I love the middleware idea and I'm planning to use it on a massive scale.
In the meantime, I noticed some problems in my test suite because globals are not cleaned up after a test.
The problem is that some libraries test for the presence of window or document to know if they run in node or in the browser.

If you take a look at what mocha-jsdom does : they have a hook to restore the global values after each test.

Another problem is react testing the ExecutionEnvironment.canUseDOM boolean.
In my experience, you can either put this to true like you did or try to initialise a fake DOM before all your test suites (with mocha --require flag for instance).
Mocha-jsdom fails miserably at this because in case of nested component re-rendering, this flag will be reinitialised.

I just wanted to open the discussion to know if you have any insight about this.

@zackify
Copy link
Member

zackify commented Sep 18, 2015

I had noticed a few of these issues when I made #6. I don't see what problems would be caused with global variables, at least so far. The only things that are global is React, window, and document. So I'm a little unsure about what you want cleaned up after a test runs.

As for the canUseDom, I found it's the only way when using jsdom inside of the library because for some reason imports aren't always loaded in order (or something else weird is happening). This library is meant to run inside of node v4+ not in the browser.

Hope that helped answer some things. Btw, thank you so much for wanting to use this library :) It makes testing so much faster for me, and I would gladly add anything to it that would help you too!

@geowarin
Copy link
Author

Thanks for your quick reply!
You are right, the tests run in node 4.

However, I'm using axios to make http requests for example and I'd like to test requests made within my app in my test suite too.
So in the browser, axios is using XMLHttpRequest and in node, they use the http library.
If you look at their code, they test for presence of XMLHttpRequest, which jsdom puts in the global.

It's totally possible to work around this by unsetting specific globals for certain tests but it does not feel like the perfect solution intellectually.

@geowarin
Copy link
Author

I you want to have a look at the project I'm working on:

@zackify
Copy link
Member

zackify commented Sep 18, 2015

Thanks for the links to your tests I see what you mean. The only thing I'm confused on now is that in the test you're doing to make an http request you're not including legit tests but I'm guessing it was included in another test and the globals are still there? If so then I see the problem.

@zackify
Copy link
Member

zackify commented Sep 18, 2015

Also, I think you'll like what I just added #9 :)

Edit: that changeValues function looks really nice, you should send a PR with it as bundled middleware!

@zackify
Copy link
Member

zackify commented Sep 22, 2015

@geowarin can you specify what you need done? I'm going to close this in a few days if there's no reply

@geowarin
Copy link
Author

Hi @zackify !
I have no perfect solution for the moment.
Not being tied to a testing framework means that you have no hook to do proper cleaning.

Maybe you could optionally let the user do the jsdom init?

#9 is fantastic!
I'll push the changeValues middleware as soon as you review #19 😄

@geowarin
Copy link
Author

I've just seen #14. It seems that we are moving towards an optional jsdom init, then.

@zackify
Copy link
Member

zackify commented Sep 23, 2015

I'm thinking even though it's as easy as not including the dom.js file, I
might make this a different file "legit-tests/nodom" or something?
On Wed, Sep 23, 2015 at 02:49 Geoffroy Warin notifications@github.com
wrote:

I've just seen #14 #14. It
seems that we are moving towards an optional jsdom init, then.


Reply to this email directly or view it on GitHub
#11 (comment).

@zackify zackify closed this as completed Oct 8, 2015
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

No branches or pull requests

2 participants