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

Update qunit/jquery and fix several console erros in Firefox at Windows #172

Open
FagnerMartinsBrack opened this issue Jan 12, 2015 · 3 comments

Comments

@FagnerMartinsBrack
Copy link

When opening firebug, the window resize make some tested events execute asynchrounously outside the test context, causing unexpected errors.

To workaround this I tested against a counter value modified by the callbacks instead of executing the assertions inside each events callback.

While debugging, I managed to find the offending code that executes the events asynchrounously (

callCallback.call(this, 'idle', delegated, callbackData);
), but due to my lack of knowledge of this plugin's internals I failed to find the root cause of the problem. The code was introduced at ba76976. For now this is just a workaround.

(Can't send a pull request because everything is in master branch)

web-stories@011c1f6

@shama
Copy link
Member

shama commented Jan 13, 2015

I'm not sure I follow the issue you're describing but this part definitely looks incorrect:

current.idleTimeout = setTimeout(function() {
  callCallback.call(this, 'idle', delegated, callbackData);
}, Math.max(1, settings.transitionDuration - 100));

callCallback should likely be using the parent context but definitely not this there.

Why can't you send a pull request? You can create a pr off a branch with git checkout -b branch-name && git push origin branch-name then open a PR. Thanks!

@FagnerMartinsBrack
Copy link
Author

I found something else that may or may not be related:

At window resize, jmpress triggers the undocumented (probably internal) "reselect" method. The reselect method calls select method without using call( this ).

What is the purpose of this reselect call upon resize? This call doesn't seems to be working, but I need to understand what this is about to test it correctly.

@FagnerMartinsBrack
Copy link
Author

@shama
I was not clear about the issue, so I will try to clarify it better:

I updated qunit and jquery in order to start analyzing the tests.

After updating qunit and fixing some of the deprecated features (like QUnit.testStart syntax), qunit started uncovering several assertions being executed outside the test context. Probably due to some async code.

After reviewing the tests I realized the assertions were being called inside the callbacks. That's not ideal for the following reasons:

  • If a callback is executed twice, qunit prints that another assertion was executed outside the context (in case of an async execution) or that it was expecting x but executed y assertions.
  • If the callback is not executed, qunit prints that it expected x assertions but got none.

Those are not useful informations to catch callback/events regressions because they do not hold any meaning. To ensure regressions are catch easly, one should specify a clear message to why a test failed. In case of callbacks and events, you should print how many times that event was supposed to execute using a counter with a message. That enables you to rapidly spot failures without having to inspect the test code or debug it.

Since using a counter fixed the test failures, I opted to not dig deeper to the root cause of this problem and just rewrite the callback/events tests. It didn't actually "fixed" the problem, it served as a workaround which, as a consequence, also updated jquery and qunit to the latest version and suppressed the test failures while debugging in the browser using Firefox/Windows.

So that's why this issue (as the title says) updated qunit/jquery and fixed several console erros in Firefox at Windows.

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