Events dependency #7274

jugglinmike · 2014-03-24T18:20:57Z

Both the Slider and Rangeslider widgets depend on the events module to function correctly. The integration tests currently pass because they explicitly load the init module, which has a dependency on the events module.

This implicit dependency causes the following problems:

The modules in question cannot be cleanly loaded from AMD projects
It is possble to create broken custom builds of jQuery Mobile (for instance, by selecting only the "Slider" widget)

jugglinmike · 2014-03-24T18:57:58Z

I would like to back this with tests, but I could use a little guidance there. (I naively attempted to remove the init requirement from the appropriate integration test folder, but doing so caused the tests to hang in the browser.)

arschmitz · 2014-03-24T19:00:07Z

js/widgets/forms/rangeslider.js

@@ -9,6 +9,7 @@ define( [ "jquery",
 	"../../core",
 	"../../widget",
 	"./textinput",
+	"../../events",


@jugglinmike thanks for catching this however I think this should depend on just "../../events/touch" i dont see any references to orientationchange, throttledresize, or navigate events in either widget.

arschmitz · 2014-03-24T19:03:33Z

@jugglinmike yes that is needed to initialize the library and make autoinit work however i think what we want to test here is that it works without autoinit and the rest of the library for this we have https://github.com/jquery/jquery-mobile/tree/master/tests/unit/individual-modules these are to make sure a build with just that widget works. I think this is what we need for slider and rangeslider.

gabrielschulhof · 2014-03-24T20:02:57Z

@jugglinmike Nice catch!

jugglinmike · 2014-03-25T13:56:36Z

Thanks!

@arschmitz: In order to test this, I plan to simulate two mouse clicks on the slider/rangeslider. This doesn't seem like a unit test exactly, so I wanted to check with you to see if that's the same approach you had in mind. If so, does jQuery Mobile's test infrastructure have any tooling for pointer event simulation that I can re-use?

jugglinmike · 2014-03-25T13:57:33Z

(also: I've updated the dependency lists to specifically reference the events/touch module as recommended by @arschmitz)

arschmitz · 2014-04-24T15:00:56Z

@jugglinmike any interest in still working on tests for this? If so i'm happy to help in any way you need.

jugglinmike · 2014-04-29T23:48:52Z

@arschmitz Certainly! Could you tell me if I was on the right track with this comment?

arschmitz · 2014-04-30T00:44:15Z

@jugglinmike sorry i thought i had responded to that but clearly I didn't. I think we need two types of tests here. one similar to these tests https://github.com/jquery/jquery-mobile/tree/master/tests/unit/individual-modules to make sure the widget properly initializes on its own this covers the second problem of broken builds. Then a second test like what you mention in your comment. For something like this we generally just trigger a click with proper event data you can see similar tests using simulated mouse events here https://github.com/jquery/jquery-mobile/blob/32902786925aa7b5ddc8d2b989bfc65a5fe80def/tests/unit/slider/slider_events.js#L248

@jugglinmike if you have any other questions please just let me know.

jugglinmike · 2014-05-08T02:04:13Z

Got the first tests in. Now working on the click tests...

jugglinmike · 2014-05-10T00:52:19Z

@arschmitz I've run up against a problem: I can't reproduce this in the test environment as it stands today. The environment explicitly depends on the init module, which depends on the navigation module, which depends on the events module, which depends on the events/touch module.

This means that the change I'm proposing will be undetectable from that
environment. I've written a passing test, but it passes even without my change.
As it stands, regressions wouldn't be detected.

Some of the existing tests depend on the init module. I think those tests
should be moved to a separate document, maybe named slider_auto_init.html.

I want to hear your thoughts on that approach before I try that, though.
Whatever we decide on, we'll most likely have to do something similar for the
RangeSlider. What do you think?

arschmitz · 2014-05-10T02:31:49Z

@jugglinmike all of our normal tests explicitly depend on the init module I think we want to keep that as is at least for now. For tests which we explicitly don't want the init module, like for these tests I think we should add those to the individual module tests so in this case the additional tests would go in the slider_core and rangeslider_core files you added already. We probably want to reconsider the organization of these tests as we add more ( not just being all in a single folder as they are now but thats more or less unrelated )

jugglinmike · 2014-05-14T00:10:02Z

Alrighty @arschmitz I've added the tests to the respective _core.js files as you've suggested. I've also shuffled the commits according to what I think makes sense for master: one set for the introduction of the _core.js files (which on their own are not related to this pull request), and a separate set for the patch to the source and related "unit" test.

It's a lot of test code, so I'm sure there's plenty I got wrong in terms of structure for this project. Just let me know what needs changing!

jugglinmike · 2014-05-14T01:09:55Z

TravisCI is reporting a test error for the "TYPES=unit JQUERIES=git" build:

FAIL 5 tests executed in 3.657s, 4 passed, 1 failed, 0 dubious, 0 skipped.      
Details for the 1 failed test:
In tests/casperjs/demos.test.js:14
  Root index should redirect to demos/
    assertHttpStatus: HTTP status code is: 200
>> 
error: trueCasper Task 'casper:demos.src' took ~4125ms to run
Warning: Task "casper:demos.src" failed.� Use --force to continue.
Aborted due to warnings.

This looks unrelated. Can you verify, @arschmitz?

arschmitz · 2014-05-14T01:39:39Z

This is indeed unrelated ill look at the tests shortly one thing i realized though is rather then depend on all of events we should be good with just vmouse since we are not using tap taphold swipe or scroll start which are the other touch events

jugglinmike · 2014-05-15T02:19:30Z

@arschmitz Updated to depend on the vmouse module as you requested. Seeing a different set of CasperJS failures on the "JQUERIES=git" build. Let me know what's next :)

arschmitz · 2014-05-15T02:22:10Z

@jugglinmike awesome im going to pull this local and review it in detail ill let you know if i see anything.

arschmitz · 2014-05-15T02:23:35Z

tests/unit/individual-modules/rangeslider_core.js

+	var rangeslider = $( "#plain" ).rangeslider();
+	var handles = rangeslider.parent().find( ".ui-slider-handle" );
+	var track = handles.closest( ".ui-slider-track" );
+	var offset = handles.offset();


Use a single car statements and comma separate each variable on a separate line.

jugglinmike · 2014-05-15T11:25:09Z

tests/unit/individual-modules/rangeslider_core.js

+		handle.trigger(down);
+		handle.trigger(move);
+		handle.trigger(up);
+	};


Also: this semicolon is unnecessary. I'll remove it.

jugglinmike · 2014-05-15T11:33:20Z

Thanks for the feedback, @arschmitz! I think I've addressed it all. I haven't squashed the changes in case that helps you review, but I'm happy to do so at your request.

arschmitz · 2014-05-15T12:09:59Z

tests/unit/individual-modules/rangeslider_core.js

+		track = handles.closest( ".ui-slider-track" ),
+		offset = handles.offset();
+
+	deepEqual( rangeslider.rangeslider( "widget" ).hasClass( "ui-rangeslider" ), true, "Has class ui-rangeslider" );


max line length 100chars

arschmitz · 2014-05-15T12:12:39Z

@jugglinmike sorry I missed those line lengths last night but iv tested local and double checked and everything looks good after these. So if you want to squash at the same ill push this to master when your done. Thanks for all the work on this!

jugglinmike · 2014-05-15T12:24:42Z

No worries, @arschmitz. I've wrapped the three lines you identified using the button_core.js tests as a guide.

arschmitz · 2014-05-15T12:39:06Z

@jugglinmike looks great if you want to squash all the commits ill push it to master

Both modules have a hard dependency on the "vmouse" module that should be expressed in AMD. Closes jquery-archivegh-7274

jugglinmike · 2014-05-15T12:57:35Z

Alright @arschmitz . Although I think this should be (at least) two separate commits, I found one example of a multi-module commit in master within the last seven months or so. I've based my commit message on that.

arschmitz · 2014-05-15T15:34:32Z

@jugglinmike ah you are right i think it should have been two commits but its fine . Thanks for the contribution!

gabrielschulhof · 2014-05-29T16:03:08Z

The commit message should also say "Re gh-5987".

Both modules have a hard dependency on the "vmouse" module that should be expressed in AMD. Re jquery-archivegh-5987 Closes jquery-archivegh-7274

jugglinmike · 2014-05-29T16:07:47Z

@gabrielschulhof Done. Think this is ready to merge?

arschmitz · 2014-05-29T16:19:28Z

@jugglinmike it is ready i already pulled rebased tested and added the re just apparently never pushed it ( it was late at night lol ) ill push it right now

coveralls · 2014-05-29T18:26:37Z

Coverage increased (+0.21%) when pulling b5fe86d on jugglinmike:events-dependency into 7b818d8 on jquery:master.

jugglinmike · 2014-05-31T01:05:19Z

@arschmitz Another late night, maybe? :P

jaspermdegroot · 2014-06-02T10:25:12Z

@arschmitz - To which branch did you push this?

For the commit message: merging will fix #7383

Both modules have a hard dependency on the "vmouse" module that should be expressed in AMD. Re gh-5987 Closes gh-7383 Closes gh-7274 (cherry picked from commit 44e2d98)

Both modules have a hard dependency on the "vmouse" module that should be expressed in AMD. Re jquery-archivegh-5987 Closes jquery-archivegh-7383 Closes jquery-archivegh-7274

gseguin added Component: Slider labels Mar 24, 2014

arschmitz reviewed Mar 24, 2014
View reviewed changes

gseguin added this to the 1.4.3 milestone Mar 24, 2014

arschmitz mentioned this pull request May 10, 2014

Slider: depend on vmouse #7384

Closed

arschmitz reviewed May 15, 2014
View reviewed changes

jugglinmike reviewed May 15, 2014
View reviewed changes

arschmitz reviewed May 15, 2014
View reviewed changes

jugglinmike added a commit to jugglinmike/jquery-mobile that referenced this pull request May 15, 2014

Slider and Rangeslider: Express module dependency

a69fef7

Both modules have a hard dependency on the "vmouse" module that should be expressed in AMD. Closes jquery-archivegh-7274

gabrielschulhof added the Action: PR needs reviewing label May 29, 2014

Slider and Rangeslider: Express module dependency

b5fe86d

Both modules have a hard dependency on the "vmouse" module that should be expressed in AMD. Re jquery-archivegh-5987 Closes jquery-archivegh-7274

jaspermdegroot mentioned this pull request Jun 2, 2014

Slider: Does not work on its own (custom build with only slider) #7383

Closed

arschmitz closed this in 44e2d98 Jun 2, 2014

jaspermdegroot removed the Action: PR needs reviewing label Aug 20, 2015

Events dependency #7274

Events dependency #7274

Uh oh!

Conversation

jugglinmike commented Mar 24, 2014

Uh oh!

jugglinmike commented Mar 24, 2014

Uh oh!

arschmitz Mar 24, 2014

Choose a reason for hiding this comment

Uh oh!

arschmitz commented Mar 24, 2014

Uh oh!

gabrielschulhof commented Mar 24, 2014

Uh oh!

jugglinmike commented Mar 25, 2014

Uh oh!

jugglinmike commented Mar 25, 2014

Uh oh!

arschmitz commented Apr 24, 2014

Uh oh!

jugglinmike commented Apr 29, 2014

Uh oh!

arschmitz commented Apr 30, 2014

Uh oh!

jugglinmike commented May 8, 2014

Uh oh!

jugglinmike commented May 10, 2014

Uh oh!

arschmitz commented May 10, 2014

Uh oh!

jugglinmike commented May 14, 2014

Uh oh!

jugglinmike commented May 14, 2014

Uh oh!

arschmitz commented May 14, 2014

Uh oh!

jugglinmike commented May 15, 2014

Uh oh!

arschmitz commented May 15, 2014

Uh oh!

arschmitz May 15, 2014

Choose a reason for hiding this comment

Uh oh!

jugglinmike May 15, 2014

Choose a reason for hiding this comment

Uh oh!

jugglinmike commented May 15, 2014

Uh oh!

arschmitz May 15, 2014

Choose a reason for hiding this comment

Uh oh!

arschmitz commented May 15, 2014

Uh oh!

jugglinmike commented May 15, 2014

Uh oh!

arschmitz commented May 15, 2014

Uh oh!

jugglinmike commented May 15, 2014

Uh oh!

arschmitz commented May 15, 2014

Uh oh!

gabrielschulhof commented May 29, 2014

Uh oh!

jugglinmike commented May 29, 2014

Uh oh!

arschmitz commented May 29, 2014

Uh oh!

coveralls commented May 29, 2014

Uh oh!

jugglinmike commented May 31, 2014

Uh oh!

jaspermdegroot commented Jun 2, 2014

Uh oh!

Uh oh!