[webappsec-testsuite] RE: Your submitted test

Copying public-webappsec-testsuite@w3.org.  

Thanks, Odin, for the great review.  I'm adding the public testsuite list for the group here as it is a good example of what we need to be doing!

1. Check in with the test coordinators when submitting tests
2. Code review
3. Better documentation

The issues you point out are from some of the first PHP I've ever written and I'm still learning the test framework by example, so I'm not surprised my code is crap! :)  I'll work on addressing the issues you point out below, follow up here on the list, and I hope others can learn a bit from this, as well.

-Brad

> -----Original Message-----
> From: Odin Hørthe Omdal [mailto:odinho@opera.com]
> Sent: Thursday, February 02, 2012 2:05 AM
> To: Hill, Brad
> Subject: Your submitted test
> 
> Hey!
> 
> I've looked through your test. As you haven't announced a call for reviews of
> it I'll just send you some directly, if you don't mind :-)
> 
> 
> Issue 1
> -------
> All the code in the beginning is unnecessary to keep in PHP. The place where
> you make the URLs etc. You are using them two places: $get_origin, and
> $next_script + urlencode($get_origin).
> 
> You can make the crossdomain path in javascript like this:
> 
> --
> // First we need a dirname function
> function dirname(path) {
>        return path.replace(/\/[^\/]*$/, '/') }
> 
> // Then substitude our address
> var crossdomain = dirname(location.href)
>        .replace('://www.', '://www2.')
> --
> 
> Then you can use:
>    req.open('get', crossdomain + 'support/get-origin.php'); and
>    req.open('get', 'support/redir-to-get-origin.php?url=' +
> encodeURIComponent(crossdomain + 'support/get-origin.php'));
> 
> 
> The benefit? It'll be easier to see what's happening from the html file, and it'll
> also work on other setups rather than hardcoding the paths.
> 
> 
> Issue 2
> -------
> You're making two tests (or actually 4), but they are sorely dependent on
> eachother. Testing this with the wrong URL's on my local setup, the first one
> failed, but the second one passed because obviously it didn't return Origin ==
> "http://www.w3c-test.org". The second test is dependent on the first one
> working. It's all really one test.
> 
> Also, you should use the async test system, not sync tests here. So start
> with:
> 
> var t = async_test();
> 
> and use that single test for everything. It's kinda similar to the review I did of
> Microsofts IndexedDB tests:
> http://lists.w3.org/Archives/Public/public-webapps-

> testsuite/2012Jan/0002.html
> 
> 
> Issue 3
> -------
> Don't use try-catch, use the framework.
> 
> t.step(function() {
>    might_throw()
> })
> 
> Does the same thing as
> 
> try {
>    might_throw()
> }
> catch(ex) {
>    assert_true(false, ex.message)
> }
> 
> Only much better! :-)
> 
> You should wrap your whole testing inside the testing framework, and for
> new functions, should do it to them to. So:
> 
> req2.onreadystatechange = t.step_func(function() {
>    whatever();
> });
> 
> That way any exceptions caught will fail the test.
> 
> 
> 
> 
> 
> As for what the test is testing itself, it isn't very clear to me. It seems wrong
> according to spec. When you do:
> 
>    local redirect to crossdomain page,
> 
> the origin-header sent to that crossdomain page should in fact be
> "www.w3c-tests.org", whilst you are testing that it is NOT that. Seems
> inverted. Care to explain?
> --
> Odin Hørthe Omdal · Core QA, Opera Software · http://opera.com /

Received on Thursday, 2 February 2012 18:26:11 UTC