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

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
6 participants
Member

jaubourg commented Dec 5, 2012

Summoned @Krinkle and OMG-Your-Commit-Sucks(tm) happened. So we're going through a proper PR this time to get everything fixed ;)

jaubourg and others added some commits Dec 4, 2012

@jaubourg jaubourg Organizes the php scripts used for testing better, so that the whole …
…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.
5824b13
@jaubourg jaubourg Fixes spacing 4008a98
@gibson042 @jaubourg gibson042 228ab3d followup 1: fix test failures. Close gh-1056. c077035

@gibson042 gibson042 commented on the diff Dec 5, 2012

test/unit/manipulation.js
@@ -2148,7 +2148,7 @@ test( "html() - script exceptions bubble (#11743)", function() {
}, "exception bubbled from inline script" );
raises(function() {
- jQuery("#qunit-fixture").html("<script src='data/badcall.js'></script>");
+ jQuery("#qunit-fixture").html("<script src='" + service("echo?content=undefined()")+ "'></script>");
@gibson042

gibson042 Dec 5, 2012

Member

I missed this one in the echoecho/ sweep.

@jaubourg

jaubourg Dec 5, 2012

Member

Yeah, we probably should use direct scripts a-la echo.php. Would simplify the folder structure anyway. I wanted to use a service-oriented approach to make it more readable, but it's actually making it worse.

@Krinkle

Krinkle Dec 5, 2012

Member

@gibson042 Thanks, fixed in my upcoming PR. Made sure there are no other ones we missed:

dev/jquery (master)$ ack 'service.*echo[^\/]' --js

(0 matches)

Owner

dmethvin commented Dec 11, 2012

Is this superceded by gh-1060 then?

Member

Krinkle commented Dec 11, 2012

@dmethvin No, gh-1060 is on top of this pull (it goes from Krinkle/.. to jquery/ajax-unit, this PR goes from jquery/ajax-unit to jquery/master). This PR depends on gh-1060.

Member

mikesherov commented Dec 21, 2012

What's the status of this pull? @jaubourg ?

Member

jaubourg commented Dec 27, 2012

I'm still waiting on @Krinkle on this one. Apparently, some config needs to be addressed in the swarm. I need to merge gh-1060 in and, if Krinkle won't separate echo and headers stuff, I will. I'd want this in before re-factoring ajax for 2.0.

Owner

dmethvin commented Jan 4, 2013

Same as with gh-1060, I'll close this until the swarm/ngnix issues are dealt with.

dmethvin closed this Jan 4, 2013

Member

jaubourg commented Jan 4, 2013

Makes me very very very sad. I invested a lot of time in this and still think it should land before we have 1.9 and 2.0 far from one another from a source code point of view.

Member

Krinkle commented Jan 4, 2013

I also invested a lot of time in the rewrite. But this isn't the end.

It can't work if it can't work. But as soon as it can work, we can get this finished in no time. And... ^H^H^H 🔔 hold on, got an idea on what the nginx problem could be..!

BRB

Member

rwaldron commented Jan 4, 2013

... I'm dying to find out what happens next!

Member

jaubourg commented Jan 4, 2013

yeah, suspens is killing me...

Member

Krinkle commented Jan 5, 2013

🎉 Okay, I haven't fully figured it out yet. But I got a lot of green lights when fixing this rule in jq03.jquery.com:/etc/nginx/conf.d/swarm.conf:

    # Support Apache/PHP-like PATH_INFO for the jQuery unit tests that use this
    rewrite ^(.*)data/jsonp.php.*$ $1data/jsonp.php;

(the paths apply only to the current test suite, not the files we're working on in this PR)

On Apache this is natively supported, but on Nginx and/or php-fpm this needs some additional configuration. Preferably we'd come up with a clean way to support it that doesn't involve hardcoding paths?:

This fixed various tests that were previously broken. Not sure if it'll fix all of them, we'll see!

Member

jaubourg commented Jan 5, 2013

That's awesome :)

Member

rwaldron commented Jan 5, 2013

💥 nice work

timmywil deleted the ajax-unit branch Jan 21, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment