Ajax tests: Refactor tests and fix bugs. #1060

Krinkle · 2012-12-06T12:46:26Z

jaubourg · 2012-12-06T13:00:14Z

test/unit/ajax.js

 			},
 			contentType: false,
 			success: function( data ) {
-				strictEqual( data, "content-type: \n", "Test content-type is not sent when options.contentType===false" );
+				deepEqual( data.headers, [], "Test content-type is not sent when options.contentType===false" );


This should be an empty object for consistency.

PHP has no distinction between associative arrays and numerical arrays. It is a very blurry thing. Either way, when an "intended to be" associative array is empty, json_encode outputs an array, not an object.

I know, but client-side unit tests shouldn't have to be dependant on a flaw in the PHP implementation of json_encode.

Agreed. Fixed on the PHP side by casting the (potentially empty) associative array to a stdClass object. That way json_encode will always encode it to a plain object.

BTW, I'm just going to leave this here:

json_encode(array(), JSON_FORCE_OBJECT);

I knew about JSON_FORCE_OBJECT but the solution I went with in the next amendment (casting (object)) is basically exactly the same (people commonly doing that lead to PHP implementing JSON_FORCE_OBJECT natively as of PHP 5.3).

I guess I'm still getting used to the fact that PHP 5.3+ is okay nowadays :)

It's been forever. 5.3 is fine! closures, namespaces, late static binding.... huzzah!

jaubourg · 2012-12-06T13:22:25Z

Nice overall but I think you went a step too far when merging the request headers logic in echo/index.php. See the comment there.

Krinkle · 2012-12-06T13:55:42Z

@jaubourg I think it makes perfect sense to use valid JSON transportation protocols for this. There are already tests before this that assert that stuff like that is working fine. Once past that we can just use this.

We also use hasClass, addClass, remove, load and what not in unit tests.

Also, the other reason I did this is to fix the "Content MIME type" warnings I was getting in Chrome. Those are all gone now.

As for the content type changing based on which headers are passed, that's not true in the latest version. The logic is like this:

echo.php responds with whatever content-type it is told to respond with. If none given, the default behaviour is that plain &content= echo is sent with text/plain. Any extended information (the query parameter &extend..= is set) is sent as JSON (with the proper content type). No implications, but explicit like that.

Note that the parameter is extend[headers][] not ?headers.

I don't see how it makes a difference whether it is in the same script or not. Instead of the content-type changing between "headers/request" and "echo" it now changes between "echo?content" and "echo?extended".

I even thought about merging the other two in as well. For the most part they are already obsolete, I just hadn't given it the time yet.

jaubourg · 2012-12-06T14:17:00Z

jQuery.load() is tested like any other method in the ajax module. hasClass, addClass and remove are not implemented "behind" $.ajax() so they are orthogonal to it. The json and jsonp logic are actually far far "behind" $.ajax() which makes all the difference in the world.

Unit tests are units, they should not test more than what they are supposed to test (and, yes, we have some further cleanup to do). They also should be independant on whether a previous test passed or not (keep aside most extreme cases like Deferreds being broken). If you have implicit json responses, then you test a whole bunch of the internals of $.ajax() each time you wanna test something completely unrelated (headers). You created a hidden dependency in the javascript unit test because of some logic in PHP.

That's why I started the rewrite: things had gone very wrong because too big a proportion of the logic of our ajax tests was taking place in PHP due to "magical" scripts that would do one thing or another, implicit changes to the response given an unrelated parameter. They created hidden dependencies client-side, they were also hidding actual problems in our unit tests.

The fact we had a lot of PHP scripts that served no purpose is one thing and good riddance. But the goal here is to empower our javascript unit tests and give them power over the whole client-server cycle. Implicit logic server-side like changing the content type because headers are supposed to be set is a no-go.

I'm guessing the actual reason you're doing so is because it makes it simpler to test for the results using deepEqual: if so create a helper client-side that does the parsing / test but keep the content type alone, please :)

Krinkle · 2012-12-07T16:32:21Z

Some tests are still failing, this is due to Nginx/php-fpm somehow consistently serving an empty response body for some of the requests. They're all passing for me locally now.

Asked @gnarf37 to investigate this server side (filed internally as jquery/infrastructure#132)

Magic quotes: Various tests were failing because (depending on php settings) the XML string passed to echo.php would be quote-escaped. So the round tripped content was no longer valid XML. Fixed by adding a simplified version of the popular "fix magic_quotes" script and including it from the other files. Variable name eval: $requestArray =& ${"_$requestArray"}; Aside from minor security issues (accessing arbitrary _SERVER, or _ENV properties) it is rather hacky. Changed to using $_POST for POST requests, and $_GET for GET requests. Never using $_REQUEST (can contain cookies sometimes, too). For the 1 test that needed to reverse this, added an option. Header echo: The test for options.contentType wasn't working. It didn't fail in QUnit because the assertion was changed to ok() instead of strictEqual in 228ab3d. When changed back the failure was revealed. Turns out PHP doesn't expose Content-Type and Content-Length in the HTTP_ match group (it exposes any arbitrary headers, e.g. HTTP_FOO_BAR, but these those are explicitly blacklisted and only accessible from by their unprefixed key, and even then only on POST requests) See: http://php.net/manual/en/reserved.variables.server.php#110763 Path-info style ("REST-like"): When replacing preg_match( '/index.php\/([^\/\?&]+)/' .. ) with the proper $_SERVER['PATH_INFO'] for .php/<path-info> reading, a bug was uncovered where the ajax test service() was building an invalid query string. See: 228ab3d#commitcomment-2256611 XSS: Although it is just a test suite, they can run on servers and clients and XSS can bite you when least expected. Added a simple but strict/safe sanitiser for the callback name. Avoids injection of javascript. Content type: Instead of defaulting to text/plain, defaulting to the proper content-type response header for the given content. * text/javascript for JSON-P * application/json for JSON Also added charset=UTF-8 Advanced echo: To make echo a little easier and more flexible I split it into a basic mode and an advanced mode. The advanced mode, among other things, replaces the "headers/request" script. Misc: * Named success callback parameter consistently "data" instead of "msg" or "obj" etc. * Added an "assertJqXhr" function, replacing the use of ok() in its place. At the very least it should've asserted that it returned some kind of object, anyhow it should be much better now. * Changed $.each([x, y], fn(toggle, val)) where !!toggle was used as named boolean switch into more explicit: $.each({x: false, y: true}, fn (val, toggle)) * Changed conditional for 304 fail to include the isOpera factor otherwise when it does fail the only error message "this is OK to fail in Opera", which is useless outside Opera. That should only be displayed if it is failing and if it is in Opera. If either is not the case, the actual condition should be asserted

dmethvin · 2012-12-11T20:07:26Z

Where does this pull stand?

Krinkle · 2012-12-11T21:30:35Z

The code is fine afaik. We can't merge it yet because it fails on swarm.jquery.com because of issues with NGINX and/or php-fpm (several of the AJAX tests are consistently failing due to an empty response from the server). They work fine locally in Apache.

jaubourg · 2012-12-12T01:39:01Z

The code is not fine. We need to extract header requests back out of echo.php for reasons exposed in an earlier comment.

Krinkle · 2012-12-12T03:38:57Z

@jaubourg Is the concern about what causes the content-type of the response to change (echo.php / echo.php?extend vs. echo.php / headers/request.php), or the fact that it uses JSON instead of plain text?

jaubourg · 2012-12-12T04:07:15Z

The implicit change of content-type is a problem for sure. The use of an argument that pretty much bypasses the logic of echo.php and branches to the headers/request.php one is also kinda weird (why not keep the two separate as we had previously? What do we gain by using the same script?). As it stands, it's a lot of logic made non-explicit client-side. But the fact it triggers all of the implicit conversion logic within $.ajax() for tests that shouldn't is the biggest issue here.

We should probably go back to a separate script (so that it makes the intent clear client-side -- name of the service in url === server-side action -- and reduces implicit server-side logic) that outputs JSON as text/plain and then handle the JSON parsing manually in each test (so that $.ajax()'s internal conversion logic is not used). The fact the headers info is output as JSON is awesome (it's much clearer that way), let's just make sure it is not automagically converted by $.ajax() because of an implicit change in content-type (too much code triggered inside the ajax architecture through server-side logic, especially for simple headers tests).

I'm just trying to keep client-side code in charge here while having tests as orthogonal and independant from one another as possible.

dmethvin · 2013-01-04T02:57:22Z

I'll close this until the issues with testswarm/ngnix/moonphase are resolved. It would be great to land a cleanup like this at some point, or figure out a way to use our own nodejs "server" for ajax testing.

Krinkle · 2013-03-07T19:04:21Z

@EliSnow Once this on lands, you're up!

@dmethvin Please re-open.

dmethvin · 2013-03-07T19:12:25Z

This must have gone stale at this point, I'll reopen but it will need some airing out.

dmethvin · 2013-03-25T15:20:11Z

Okay, closing this into obscurity unless someone rescues it.

mgol · 2013-04-07T05:12:03Z

@dmethvin, you said you're closing this but you didn't. :)

Krinkle · 2013-04-07T05:18:40Z

Why the rush? I'll get to it, I promise. There's been a lot of effort put into this. Archiving it now will probably mean we forget about it and waste efforts a few months from now going through he same arguments.

dmethvin · 2013-05-27T16:35:42Z

It's been 6 months on this... @Krinkle do you have a plan/schedule for it?

mgol · 2013-08-19T16:33:29Z

@Krinkle, ping? 9 months have passed...

dmethvin · 2013-08-26T16:01:28Z

Imma close this. Glad to reopen if it gets love.

jaubourg reviewed Dec 6, 2012
View reviewed changes

Krinkle added 2 commits December 7, 2012 18:03

Ajax tests: Rename services <name>/index.php to <name>.php

cae097e

dmethvin mentioned this pull request Dec 11, 2012

No Ticket: Rationalize PHP scripts for ajax unit tests #1058

Closed

dmethvin closed this Jan 4, 2013

dmethvin reopened this Mar 7, 2013

dmethvin closed this Aug 26, 2013

Krinkle deleted the ajax-unit-fixup branch December 14, 2013 01:02

lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ajax tests: Refactor tests and fix bugs. #1060

Ajax tests: Refactor tests and fix bugs. #1060

Krinkle commented Dec 6, 2012

jaubourg Dec 6, 2012

Krinkle Dec 6, 2012

jaubourg Dec 6, 2012

Krinkle Dec 6, 2012

mikesherov Dec 7, 2012

Krinkle Dec 7, 2012

mikesherov Dec 7, 2012

jaubourg commented Dec 6, 2012

Krinkle commented Dec 6, 2012

jaubourg commented Dec 6, 2012

Krinkle commented Dec 7, 2012

dmethvin commented Dec 11, 2012

Krinkle commented Dec 11, 2012

jaubourg commented Dec 12, 2012

Krinkle commented Dec 12, 2012

jaubourg commented Dec 12, 2012

dmethvin commented Jan 4, 2013

Krinkle commented Mar 7, 2013

dmethvin commented Mar 7, 2013

dmethvin commented Mar 25, 2013

mgol commented Apr 7, 2013

Krinkle commented Apr 7, 2013

dmethvin commented May 27, 2013

mgol commented Aug 19, 2013

dmethvin commented Aug 26, 2013

Ajax tests: Refactor tests and fix bugs. #1060

Ajax tests: Refactor tests and fix bugs. #1060

Conversation

Krinkle commented Dec 6, 2012

jaubourg Dec 6, 2012

Choose a reason for hiding this comment

Krinkle Dec 6, 2012

Choose a reason for hiding this comment

jaubourg Dec 6, 2012

Choose a reason for hiding this comment

Krinkle Dec 6, 2012

Choose a reason for hiding this comment

mikesherov Dec 7, 2012

Choose a reason for hiding this comment

Krinkle Dec 7, 2012

Choose a reason for hiding this comment

mikesherov Dec 7, 2012

Choose a reason for hiding this comment

jaubourg commented Dec 6, 2012

Krinkle commented Dec 6, 2012

jaubourg commented Dec 6, 2012

Krinkle commented Dec 7, 2012

dmethvin commented Dec 11, 2012

Krinkle commented Dec 11, 2012

jaubourg commented Dec 12, 2012

Krinkle commented Dec 12, 2012

jaubourg commented Dec 12, 2012

dmethvin commented Jan 4, 2013

Krinkle commented Mar 7, 2013

dmethvin commented Mar 7, 2013

dmethvin commented Mar 25, 2013

mgol commented Apr 7, 2013

Krinkle commented Apr 7, 2013

dmethvin commented May 27, 2013

mgol commented Aug 19, 2013

dmethvin commented Aug 26, 2013