Tests: Replace resource loader with AMD #1335

rxaviers · 2014-09-02T20:47:27Z

💥 WIP...

Pulll request's goal is to AMDify tests for 1.12.0 release and address part of http://bugs.jqueryui.com/ticket/10119.

TODO:

rxaviers · 2014-09-02T20:48:09Z

Initial PR based on #1029

rxaviers · 2014-09-02T20:49:25Z

external/require.js

+                    //registered for it. Start with most specific name
+                    //and work up from it.
+                    for (i = syms.length; i > 0; i -= 1) {
+                        parentModule = sy


Need to use bower + bower_copy to place this file.

rxaviers · 2014-09-02T20:52:12Z

Better suggestions for the commit message? Actual: "Tests: Replace resource loader with AMD"

mikesherov · 2014-09-02T20:58:28Z

tests/unit/accordion/accordion.html

+			"ui": "../../../ui"
+		},
+		shim: {
+			"jquery.simulate": [ "jquery" ]


What did we ever decide for jquery-color as far as UMD goes? The same would apply here to jquery-simulate. /cc @gnarf

I would just follow what UI has, unless you think it's reasonable to use in node.

mikesherov · 2014-09-02T20:59:54Z

Don't indent inside the define please. Creates an unnecessarily high number of changes, and is against the style guide: http://contribute.jquery.org/style-guide/js/#full-file-closures

rxaviers · 2014-09-02T21:21:38Z

Don't indent inside the define please...

Absolutely... Fixed.

scottgonzalez · 2014-09-02T22:11:09Z

Gruntfile.js

 				"qunit/MIT-LICENSE.txt": "qunit/MIT-LICENSE.txt",
+				"qunit/qunit.css": "qunit/qunit/qunit.css",
+				"qunit/qunit.js": "qunit/qunit/qunit.js",
+				"requirejs/require.js": "requirejs/require.js",


requirejs should be visually split from qunit.

Any preference on how to order it between qunits and jqueries? They are not in sort order... Should I use dependencies sort order, than jqueries? Whatever order is fine?

mikesherov · 2014-09-02T22:11:59Z

Still looks like a huge number of changes. Please squash the commits so that there aren't entries in the history having them changed and changed back.

rxaviers · 2014-09-02T22:13:40Z

All fix commits are going to be squashed before merged, not now to save history in this PR though.

scottgonzalez · 2014-09-02T22:15:59Z

tests/unit/accordion/accordion.html

@@ -134,6 +116,29 @@ <h3 class="bar">Rent one bear, ...</h3>
 	</dd>
 </dl>

+	<script src="../../../external/requirejs/require.js"></script>


Why did all of this move down? It's not even the end of the body, it's in the fixture.

This was supposed to be in the end of the body, not in the fixture div (needs fix). Anyway, the reason for moving it down was to favor rendering of other elements of the page before loading and executing these scripts.

scottgonzalez · 2014-09-03T11:58:56Z

tests/unit/accordion/accordion.html

+	<script src="../helper/jquery.js"></script>
+	<script>
+	QUnit.config.autostart = false;
+	require.config({


I assume we'd have the config defined somewhere else for re-use.

Yeap, this will eventually get wrapped into a helper or something.

mikesherov · 2014-09-23T13:59:00Z

The next Qunit will no longer rely on test loading order for rerun functionality. We should wait till that lands and then re-revert the changes so we don't have to wrap all tests in an a fucntion to be executed later.

jzaefferer · 2014-10-23T14:26:49Z

I just tried to test this, by checking out the PR and opening tests/unit/accordion/accordion.html. It gets stuck on "Running...", with this exception on the console:

Regarding execution order, the require.js docs say this:

RequireJS waits for all dependencies to load, figures out the right order in which to call the functions that define the modules, then calls the module definition functions in the right order.

So it shouldn't matter in what order the file content is loaded, in the end they're executed "in the right order". Since it doesn't define the semantics of "right order", this may not be what we're looking for. Since the current setup in this PR is broken, I can't really verify it here.

jzaefferer · 2014-11-07T17:13:17Z

I rebased this branch against master and fixed the failure (switching two lines, not sure how that happened).

I've investigated if there's some way to load modules in the specified order without extra workarounds. The order only depends on other dependencies, but loading several files that have to dependencies to each other results in seemingly random order. Its actually not random, as far as I can tell its in the specified order, except when one of the modules loads a dependency not needed by the others, so here "accordion_core" is executed last, since it also loads "jquery-simulate".

Anyway, one alternative to what @rxaviers implemented so far is nesting require calls like this:

require(["./accordion_common"], function() {
    require(["./accordion_core"], function() {
        require(["./accordion_events"], function() {
            require(["./accordion_methods"], function() {
                require(["./accordion_options"], function() {
                    QUnit.start();
                })
            })
        })
    })
});

I'm hoping there's a better way.

The above at least requires less modifications of our test files (no need for the separate wrapper) and could probably be abstracted so that we can still do this:

requireTests([
    "./accordion_common",
    "./accordion_core",
    "./accordion_events" ,
    "./accordion_methods",
    "./accordion_options"
], function() {
    QUnit.start();
});

This was supposed to be in the end of the body, not in the fixture div

This reverts commit baa3378.

Cleanup be3b2b2

- Port autocomplete;

rxaviers · 2014-11-14T15:52:09Z

Updates:

Moved common require.config to a helper. It allows simplification (see 337b775) of each tests/unit/<component>/<component>.html.
Wrapped "using nested requires" into require_series, so it's saner using it.
By using nested requires, we avoid the extra indentation on tests/unit/<component>/<component>_<test_module>.js files and we get a better (more concise) diff (example).

- Port all others;

rxaviers · 2014-11-18T20:10:22Z

Update, I'm in the process of updating the tests of all other components. The problem I'm facing is that I get intermittent failures running datepicker tests tests/unit/datepicker/datepicker.html...

rxaviers · 2014-11-18T20:18:51Z

tests/unit/testsuite.js

-		onFocus();
-	};
-
-	element.bind( "focus", fn )[ 0 ].focus();


Just curious. Should we replace [ 0 ] with .first()?

@c43500 Please reply using GitHub; your email replies have no formatting.

@rxaviers No. Doing that will likely break tests because you would be doing something different (triggering a focus event through jQuery instead of natively focusing the element).

Got it. Thanks

- Port all others;

rxaviers · 2014-11-19T12:13:11Z

Update. All tests are running. Some are failing (including the intermittent one described above). Need to:

Figure out why the remaining ones are failing and fix them.
Make sure swarminject is working.

jzaefferer · 2014-11-19T17:12:02Z

To test swarminject: Log into Jenkins, create new job, pick some option to make it a copy of the existing jQuery UI (master) job, change branch from master to AMDify-tests, disable build triggers, save, then start a manual run.

jzaefferer · 2014-11-20T14:47:04Z

Can we smash all the files in test/unit/helper/ into one js file? I don't see the point in splitting those up when we use them on every page. And you can get rid of the window.require dependency by exporting the right variable: http://requirejs.org/docs/api.html#config

var require = { ... };

Would also remove the need to export window.helper.jqueryUrl at all, since you could just use that directly.

One issue I noticed when running datepicker tests: The order isn't consistent. JSHint tests used to run first, now they run at some point later, whatever the logic for that. I guess the problem is the require call inside tests/unit/helper/testsuite.js, which then defines another test.

I also get many errors on every other run of the datepicker tests. There seems to be a problem with expect() calls, which probably means that QUnit's globals aren't available - expect() is always the first method called in each test.

Not sure yet what exactly is going on, hope that helps. Should have more time later.

- Port all others;

rxaviers · 2014-11-21T14:03:45Z

Sure, we can smash the helpers now that we know where each is used. Note helper/css.js is the only one needed in the header before style loading. I'd encourage having that one separate. (and I guess you meant window.helper instead)

I've updated the description with the remaining TODOs.

- Port all others;

rxaviers · 2014-11-26T17:19:01Z

tests/unit/helper/testsuite.js

+		return;
+	}
+
+	require([ "jshint" ], function() {


Remove this require and instead have "jshint" under the test definition.

That would load this when we don't need it. Just swap this with the line below.

Just swap this with the line below.

rxaviers reviewed Sep 2, 2014
View reviewed changes

mikesherov reviewed Sep 2, 2014
View reviewed changes

scottgonzalez reviewed Sep 2, 2014
View reviewed changes

rxaviers referenced this pull request Sep 2, 2014

Demos: Use AMD loader

0738b99

scottgonzalez reviewed Sep 3, 2014
View reviewed changes

rxaviers force-pushed the AMDify-tests branch 2 times, most recently from a113a9e to fc133d5 Compare September 3, 2014 14:05

jzaefferer force-pushed the AMDify-tests branch from e52a429 to 97813f7 Compare November 7, 2014 17:02

rxaviers added 4 commits November 14, 2014 10:09

(fix) Tests: Replace resource loader with AMD

2834651

This was supposed to be in the end of the body, not in the fixture div

Revert "(fix) Tests: Replace resource loader with AMD"

8ac5a49

This reverts commit baa3378.

(fix) Tests: Replace resource loader with AMD

b2d649f

Cleanup be3b2b2

(fix) Tests: Replace resource loader with AMD

cf94731

Cleanup be3b2b2

rxaviers force-pushed the AMDify-tests branch from 97813f7 to cf94731 Compare November 14, 2014 12:09

rxaviers added 3 commits November 14, 2014 11:27

(fix) Tests: Replace resource loader with AMD

21bbc2e

(fix) Tests: Replace resource loader with AMD

337b775

(fix) Tests: Replace resource loader with AMD

cdfc27e

- Port autocomplete;

(fix) Tests: Replace resource loader with AMD

9c3d95c

- Port all others;

rxaviers reviewed Nov 18, 2014
View reviewed changes

rxaviers added 3 commits November 19, 2014 09:20

(fix) Tests: Replace resource loader with AMD

bc593d3

- Port all others;

(fix) Tests: Replace resource loader with AMD

7655302

- Port all others;

(fix) Tests: Replace resource loader with AMD

64b786f

- Port all others;

rxaviers added 2 commits November 21, 2014 11:23

(fix) Tests: Replace resource loader with AMD

b810fc3

- Port all others;

(fix) Tests: Replace resource loader with AMD

757164d

- Port all others;

(fix) Tests: Replace resource loader with AMD

8281580

- Port all others;

rxaviers reviewed Nov 26, 2014
View reviewed changes

(fix) Tests: Replace resource loader with AMD

f44cb7c

Just swap this with the line below.

jquerybot added the CLA: Valid label Feb 26, 2015

jzaefferer added this to the land-before-esformatter milestone Mar 13, 2015

scottgonzalez mentioned this pull request Mar 18, 2015

Tests: Replace resource loader with AMD #1511

Closed

10 tasks

scottgonzalez closed this Mar 25, 2015

jzaefferer deleted the AMDify-tests branch May 15, 2015 11:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests: Replace resource loader with AMD #1335

Tests: Replace resource loader with AMD #1335

rxaviers commented Sep 2, 2014

rxaviers commented Sep 2, 2014

rxaviers Sep 2, 2014

rxaviers commented Sep 2, 2014

mikesherov Sep 2, 2014

scottgonzalez Sep 3, 2014

mikesherov commented Sep 2, 2014

rxaviers commented Sep 2, 2014

scottgonzalez Sep 2, 2014

rxaviers Sep 2, 2014

mikesherov commented Sep 2, 2014

rxaviers commented Sep 2, 2014

scottgonzalez Sep 2, 2014

rxaviers Sep 2, 2014

scottgonzalez Sep 3, 2014

rxaviers Sep 3, 2014

mikesherov commented Sep 23, 2014

jzaefferer commented Oct 23, 2014

jzaefferer commented Nov 7, 2014

rxaviers commented Nov 14, 2014

rxaviers commented Nov 18, 2014

rxaviers Nov 18, 2014

rxaviers Nov 18, 2014

scottgonzalez Nov 18, 2014

scottgonzalez Nov 18, 2014

rxaviers Nov 18, 2014

rxaviers commented Nov 19, 2014

jzaefferer commented Nov 19, 2014

jzaefferer commented Nov 20, 2014

rxaviers commented Nov 21, 2014

rxaviers Nov 26, 2014

scottgonzalez Nov 26, 2014

Tests: Replace resource loader with AMD #1335

Tests: Replace resource loader with AMD #1335

Conversation

rxaviers commented Sep 2, 2014

rxaviers commented Sep 2, 2014

Choose a reason for hiding this comment

rxaviers commented Sep 2, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikesherov commented Sep 2, 2014

rxaviers commented Sep 2, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikesherov commented Sep 2, 2014

rxaviers commented Sep 2, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikesherov commented Sep 23, 2014

jzaefferer commented Oct 23, 2014

jzaefferer commented Nov 7, 2014

rxaviers commented Nov 14, 2014

rxaviers commented Nov 18, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rxaviers commented Nov 19, 2014

jzaefferer commented Nov 19, 2014

jzaefferer commented Nov 20, 2014

rxaviers commented Nov 21, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment