freedomjs / freedom

Embracing a distributed web
http://freedomjs.org
Apache License 2.0
512 stars 53 forks source link

oauth.remotepageauth doesn't close tabs successfully in Firefox #292

Open agallant opened 8 years ago

agallant commented 8 years ago

This test times out (doesn't get to the done call). Tweaking timeout seemed to improve it for a bit, but not reliably so. Disabling all other integration tests does allow it to pass, however.

The failing test is: https://github.com/freedomjs/freedom/blob/soycode-fixstoragetest/spec/providers/storage/storage.isolated-shared.integration.spec.js#L30-L51

And the method where it looks to hang is: https://github.com/freedomjs/freedom/blob/soycode-fixstoragetest/spec/util.js#L291-L302

Firefox 39.0.0 (Ubuntu 0.0.0) WARN '%c[Module Shared Storage]%c', 'color: red', 'color: none',
'[unbound Provider] dropping message {"to":13,"type":"message","message"
{"action":"method","type":"set","reqId":1,"text":["outerKey","outerValue"],"binary":[]}}'

That is, it doesn't even get into the first callback (the test is a sequence of 5 callbacks).

agallant commented 8 years ago

I think it is the oauth test somehow "leaking over", as it does run prior - https://github.com/freedomjs/freedom/blob/2ed5badc8ee567f571a6ee7aa712990eed1bc550/spec/providers/coreIntegration/oauth.integration.src.js#L63-L72

agallant commented 8 years ago

The issue may be that the test isn't successfully closing the tab for "error.com", and so the remaining test is running in the wrong scope. Per @dborkan the tab should be closed by: https://github.com/freedomjs/freedom/blob/master/providers/oauth/oauth.remotepageauth.js#L90

agallant commented 8 years ago

The current approach doesn't seem to close the tab in Chrome either, it's just Chrome continues running the tests properly and ultimately closes the whole browser after the tests complete. I've tried a few variants of window.close and similar without success.

willscott commented 8 years ago

Are you sure this isn't a bug associated with the longstanding issue on stability of isolated storage: https://github.com/freedomjs/freedom/issues/77

agallant commented 8 years ago

Maybe, but it is occurring very regularly post-Dan's commits and not so regularly pre, and the test does still pass if you disable other tests.

agallant commented 8 years ago

Also, from experimentation, tests pass if you manually monitor the Firefox browser and close the "error.com" tabs that are opened. So, either (a) the test needs to successfully close those tabs, or (b) the test needs to be refactored to not require opening new tabs. The former seems like it shouldn't be too hard, but various typical invocations of window.close are not having much luck.

agallant commented 8 years ago

I do think this is an actual issue with the oauth.remotepageauth provider, and that getting it to close tabs successfully will fix the test(s). I'll still work on it some, but assigning to Dan since he worked on that provider recently and has the most familiarity with the new code.

So far the approaches I've tried either don't close the tab, or close the wrong (parent/original) tab. Trying to keep a reference to the window opened doesn't seem to work as expected either.

agallant commented 8 years ago

For now I've disabled the test, so it's clear the build is working. Again, all tests pass when run individually (including this one), this test just needs to successfully close tabs so the remaining test(s) can pass. I do want to re-enable the test in the near future, Dan lmk thoughts/if you want to chat about how to fix. Thanks!

agallant commented 8 years ago

Revisiting, I think the actual issue is that this test causes Firefox to reload the page: Some of your tests did a full page reload!

Investigation suggests that Karma may be inherently against this, and a workaround (e.g. http://stackoverflow.com/questions/20029474/writing-a-test-for-a-directive-that-does-a-full-page-reload-in-karma - though this is pretty Angular specific) may be needed.