Skip to content

Commit

Permalink
Organizes the php scripts used for testing better, so that the whole …
Browse files Browse the repository at this point in the history
…logic of a unit, server-side and client-side, is contained within the unit itself. Nearly all ajax unit tests take advantage of the new 'framework'. Lots of files got deleted because they became redundant or weren't used anymore.
  • Loading branch information
jaubourg committed Dec 4, 2012
1 parent 3ab2634 commit 228ab3d
Show file tree
Hide file tree
Showing 38 changed files with 828 additions and 1,301 deletions.
6 changes: 5 additions & 1 deletion test/.jshintrc
Expand Up @@ -41,11 +41,15 @@
"ajaxTest": true,
"testIframe": true,
"testIframeWithCallback": true,
"createComplexHTML": true,

This comment has been minimized.

Copy link
@Krinkle

Krinkle Dec 5, 2012

Member

Mixed tabs and spaces..

"createDashboardXML": true,
"createWithFriesXML": true,
"createXMLFragment": true,
"moduleTeardown": true,
"testBar": true,
"testFoo": true,
"url": true,
"url": true,
"service": true,
"t": true,
"q": true,
"amdDefined": true,
Expand Down
43 changes: 43 additions & 0 deletions test/data/ajax/echo/index.php
@@ -0,0 +1,43 @@
<?php

$requestArray = "REQUEST";

if ( isset( $_REQUEST["requestArray"] ) ) {
$requestArray = $_REQUEST["requestArray"];
}

$requestArray =& ${"_$requestArray"};

This comment has been minimized.

Copy link
@Krinkle

Krinkle Dec 5, 2012

Member

This is a horrible way to simply switch between GET and POST variable retrieval.

This comment has been minimized.

Copy link
@jaubourg

jaubourg Dec 5, 2012

Author Member

OK, then I'll use an array, I see the security concern, I was deep into the tests trying to find a way to switch.


$response = array(
"status" => "200",
"statusText" => "",
"contentType" => "text/plain",
"content" => "",
"callback" => "",
"delay" => 0
);

foreach( $response as $field => &$value ) {
if ( isset( $requestArray[ $field ] ) ) {
$value = $requestArray[ $field ];
}
}

if ( is_array( $response["content"] ) ) {
$response["content"] = http_build_query( $response["content"] );
}

if ( !$response["callback"] && preg_match( '/index.php\/([^\/\?&]+)/', $_SERVER["REQUEST_URI"], $match ) ) {
$response["callback"] = $match[ 1 ];
}

header("HTTP/1.1 $response[status] $response[statusText]");

This comment has been minimized.

Copy link
@Krinkle

Krinkle Dec 5, 2012

Member
PHP Notice:  Use of undefined constant status 
PHP Notice:  Use of undefined constant statusText 
PHP Notice:  Use of undefined constant contentType
PHP Notice:  Use of undefined constant content

Keys have to be quoted, even in PHP. Please enable error_reporting during development.

This comment has been minimized.

Copy link
@jaubourg

jaubourg Dec 5, 2012

Author Member

Got you. No idea why my installation doesn't show errors though. It should.

header("Content-Type: $response[contentType]");

if ( $response["delay"] ) {
sleep( $response["delay"] );
}

echo $response["callback"]
? "$response[callback](" . json_encode("$response[content]") . ");"

This comment has been minimized.

Copy link
@Krinkle

Krinkle Dec 5, 2012

Member

Why is $response['content'] embedded in a string?

This comment has been minimized.

Copy link
@jaubourg

jaubourg Dec 5, 2012

Author Member

Because I thought I didn't need the quotes, which I no know I do.

: "$response[content]";
30 changes: 30 additions & 0 deletions test/data/ajax/headers/cache/index.php
@@ -0,0 +1,30 @@
<?php
$headers = array(

"If-Modified-Since" => array(
"request" => "HTTP_IF_MODIFIED_SINCE",
"response" => "Last-Modified",
),
"If-None-Match" => array(
"request" => "HTTP_IF_NONE_MATCH",
"response" => "Etag",
),

);

$header = $_REQUEST["header"];
$value = $_REQUEST["value"];

if ( $header === "If-None-Match" ) {
$value = md5( $value );
}

$headers = $headers[ $header ];

$requestHeader = isset( $_SERVER[ $headers["request"] ] ) ? stripslashes($_SERVER[ $headers["request"] ]) : false;

This comment has been minimized.

Copy link
@Krinkle

Krinkle Dec 5, 2012

Member

Why stripslashes()?

if ( $requestHeader === $value ) {
header("HTTP/1.0 304 Not Modified");
} else {
header("$headers[response]: $value");

This comment has been minimized.

Copy link
@Krinkle

Krinkle Dec 5, 2012

Member
PHP Notice:  Use of undefined constant response 
echo $requestHeader ? "OK: $value": "FAIL";
}
12 changes: 12 additions & 0 deletions test/data/ajax/headers/request/index.php
@@ -0,0 +1,12 @@
<?php

$headers = array();

foreach( $_SERVER as $key => $value ) {
$key = str_replace( "_" , "-" , substr($key,0,5) == "HTTP_" ? substr($key,5) : $key );
$headers[ $key ] = $value;
}

foreach( explode( "," , $_GET["headers"] ) as $key ) {
echo "$key: " . @$headers[ strtoupper( $key ) ] . "\n";
}
5 changes: 5 additions & 0 deletions test/data/ajax/headers/response/index.php
@@ -0,0 +1,5 @@
<?php

foreach( $_REQUEST as $header => $value ) {
@header("$header: $value");

This comment has been minimized.

Copy link
@Krinkle

Krinkle Dec 5, 2012

Member

Why error @ error suppression?

This comment has been minimized.

Copy link
@jaubourg

jaubourg Dec 5, 2012

Author Member

That was reminiscent of the old code.

}
4 changes: 0 additions & 4 deletions test/data/atom+xml.php

This file was deleted.

1 change: 0 additions & 1 deletion test/data/badcall.js

This file was deleted.

1 change: 0 additions & 1 deletion test/data/badjson.js

This file was deleted.

10 changes: 0 additions & 10 deletions test/data/cleanScript.html

This file was deleted.

11 changes: 0 additions & 11 deletions test/data/dashboard.xml

This file was deleted.

1 change: 0 additions & 1 deletion test/data/echoData.php

This file was deleted.

1 change: 0 additions & 1 deletion test/data/echoQuery.php

This file was deleted.

5 changes: 0 additions & 5 deletions test/data/errorWithText.php

This file was deleted.

21 changes: 0 additions & 21 deletions test/data/etag.php

This file was deleted.

1 change: 0 additions & 1 deletion test/data/evalScript.php

This file was deleted.

18 changes: 0 additions & 18 deletions test/data/headers.php

This file was deleted.

20 changes: 0 additions & 20 deletions test/data/if_modified_since.php

This file was deleted.

13 changes: 0 additions & 13 deletions test/data/json.php

This file was deleted.

1 change: 0 additions & 1 deletion test/data/json_obj.js

This file was deleted.

14 changes: 0 additions & 14 deletions test/data/jsonp.php

This file was deleted.

1 change: 0 additions & 1 deletion test/data/name.html

This file was deleted.

24 changes: 0 additions & 24 deletions test/data/name.php

This file was deleted.

12 changes: 0 additions & 12 deletions test/data/params_html.php

This file was deleted.

11 changes: 0 additions & 11 deletions test/data/script.php

This file was deleted.

5 changes: 0 additions & 5 deletions test/data/statusText.php

This file was deleted.

7 changes: 0 additions & 7 deletions test/data/test.html

This file was deleted.

3 changes: 0 additions & 3 deletions test/data/test.js

This file was deleted.

7 changes: 0 additions & 7 deletions test/data/test.php

This file was deleted.

5 changes: 0 additions & 5 deletions test/data/test2.html

This file was deleted.

3 changes: 0 additions & 3 deletions test/data/test3.html

This file was deleted.

7 comments on commit 228ab3d

@jaubourg
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Krinkle seems like we have 12 failures on all browsers in ajax units... thing is I ran those test locally against all browsers so there must be some config discrepancy. Gotta get some sleep right now, but if you have manage to guess what's going on, I'd be happy to adapt the tests to testswarm's configuration if needs be later today ;)

@Krinkle
Copy link
Member

@Krinkle Krinkle commented on 228ab3d Dec 5, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please some consistency. echo, echo/, echo/index.php. What's it gonna be?

I'd recommend either being completely explicit everywhere (echo/index.php) or abstracting it (calling service() only with a symbolic name and mapping those to a file by appending .php). For example, rename echo/index.php to echo.php. And call service("echo"), which then appends .php.

Depending on which web server (Apache, Nginx, Nodejs, ..) and configuration therefor, various of these may fail (12 fail on jQuery servers).

There various random factors being relied on:

  • accepting directory lookups without trailing slash
  • assuming that even if that is supported, that it does not use a redirect (in which case the AJAX request would get a 301 response)
  • using index.php as the (or one of) default directory index files
    etc.

In case of TestSwarm servers, it does support directory lookups without trailing slash but 301 redirects, causing some of the requests to fail when they get 301 instead of whatever response is expected.

@Krinkle
Copy link
Member

@Krinkle Krinkle commented on 228ab3d Dec 5, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is various other issues with the PHP code that are imho unacceptable (we do execute this, server side, on production servers!). Please do this from a PR next time instead of pushing straight into master.

@Krinkle
Copy link
Member

@Krinkle Krinkle commented on 228ab3d Dec 5, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

228ab3d (by @gibson042) fixes a few of the path errors but from running it locally and by actually looking at the errors on TestSwarm I see that that was only a small part of the problem. I can reproduce the majority of the issues locally regardless of any path format.

There seem to be some invalid XML responses and stuff. Not something I can imagine would make any difference on whatever web server or browser.

And there are the style/security issues with PHP code mentioned earlier (though those don't affect the tests). I don't have time for this right now, as in, we can't have this in master taking over other people's priority all of a sudden.

I recommend reverting these 2 commits out of master ASAP. Then re-submit it as a pull request to go through further review and iteration until it passes the tests.

@jaubourg
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Krinkle, there are only 12 tests not passing. I'd prefer we fix the issue on master and be done with it rather than revert/PR/commit... We can fix all the issues here with a simple commit.

If you prefer it consistent, then we can make sure the paths are always correctly formatted. I just assumed that 301 were followed and that data just weren't rePOSTed again after the redirection (hence the differences between echo, echo/, ...). I can as easily switch to a pure file.php approach, that like 2 minutes of work.

I see the request array switching as a security risk, is there any other security issue I didn't see? Also, where is the style guide for the php code? Is there any? I tried to stay consistent with the Core style guide as I didn't know of any specific to PHP.

@scottgonzalez
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, where is the style guide for the php code? Is there any? I tried to stay consistent with the Core style guide as I didn't know of any specific to PHP.

We should write one (or at least write the differences between PHP and JS). Basically, just follow the core style guide but use single quotes instead of double quotes unless you need string interpolation.

@jaubourg
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1058 as per discussion on IRC.

Please sign in to comment.