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

fix (client/karma): Set reload context flag to avoid full page reload #1648

Merged
merged 1 commit into from
Oct 22, 2015

Conversation

nebulou5
Copy link
Contributor

@GitCop
Copy link

GitCop commented Oct 21, 2015

There were the following issues with your Pull Request

  • Commit: 033caad
    • Your subject line is longer than 70 characters
    • Invalid type. Valid types are feat, fix, docs, style, refactor, test, chore, revert

Guidelines are available at http://karma-runner.github.io/0.13/dev/git-commit-msg.html


This message was auto-generated by https://gitcop.com

@dignifiedquire
Copy link
Member

Could you please explain the reasoning behind these changes? (Not saying they are wrong just having a hard time understanding it)

@nebulou5
Copy link
Contributor Author

@dignifiedquire My thoughts are that, anytime "navigateContextTo" gets called (do a search and find instances of invocation), it's a result of the framework purposely changing the context location, either based on an emitted event or otherwise, thus, resulting in an erroneous "Some of your tests did a full page reload!" when it was not a result of a unit test, but rather, an event within the test framework itself.

The 'full page reload' error was getting triggered any time I had excess describe blocks. I'm not sure why the number of describe blocks would affect the number of times this function is called. Perhaps 'execute' is getting emitted more than once? I didn't really dig deep enough to see.

@dignifiedquire
Copy link
Member

Okay thanks, merging this as it makes sense and hopefully will improve the behaviour for others :)

dignifiedquire added a commit that referenced this pull request Oct 22, 2015
fix (client/karma): Set reload context flag to avoid full page reload
@dignifiedquire dignifiedquire merged commit ad18ce3 into karma-runner:master Oct 22, 2015
@nebulou5
Copy link
Contributor Author

@dignifiedquire there is something else going on here. I'm still seeing the issue after messing with my tests some more. The suite runs as expected, but when I add any additional describe blocks with just basic expectations, true to be true, for instance, I get the full page reload error.

@megawac
Copy link

megawac commented Oct 22, 2015

Did this make it into 0.13.12?

@dignifiedquire
Copy link
Member

Yes, please test it and give feedback if it works or not

@megawac
Copy link

megawac commented Oct 22, 2015

We're still receiving the full page reload error in WebKit based browsers with 0.13.12. The tests complete with 392/392 success but fail with the reload error

Reference travis log https://travis-ci.org/jashkenas/backbone/builds/86852035

@nebulou5
Copy link
Contributor Author

@megawac I'm still getting it as well in various browsers. My testing last night must have been a fluke. I'm going to dig a bit deeper into this today. It's really affecting our ability to write tests.

@megawac
Copy link

megawac commented Oct 22, 2015

Yeah, it appears to be all webkit based browsers (karma v0.12 still works). I've tested with Chrome + chrome mobile, safari, and opera

@nebulou5
Copy link
Contributor Author

@megawac I don't think it's browser specific. It's happening in PhantomJS for us as well.

@megawac
Copy link

megawac commented Oct 22, 2015

Isn't phantom also webkit based? I haven't seen this occur with IE, Firefox, Edge, or Opera 11/12

@nebulou5
Copy link
Contributor Author

@megawac Yes it is. Interesting, let me try ours out in Firefox.

@nebulou5
Copy link
Contributor Author

@megawac I am getting the issue with firefox as well

@dignifiedquire
Copy link
Member

I think the issue is that you need to wait for the iframe to be loaded before you set the reloadingContext to false

@dignifiedquire
Copy link
Member

@megawac @FuzzySockets take a look at #1653

@nebulou5
Copy link
Contributor Author

@dignifiedquire This did it! Good team work sir.

@nebulou5
Copy link
Contributor Author

@dignifiedquire @megawac Added a fix for when iframe is configured to false: #1655

@nebulou5
Copy link
Contributor Author

Looking into the failed tests.

@nebulou5
Copy link
Contributor Author

I'm not getting the failed tests locally, hmm.

@nebulou5
Copy link
Contributor Author

Oh I see, there is a PR just before mine:

Pull Request #1654 [email protected] breaks build ⚠️
chore(package): update karma-qunit to version 0.1.6

@nebulou5
Copy link
Contributor Author

I'm starting to think this issue goes a bit deeper guys. I'm unable to trigger a full page reload error at all now.

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

Successfully merging this pull request may close these issues.

4 participants