Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Get tests to use echoecho delays #648

Closed
wants to merge 8 commits into from

Conversation

ekashida
Copy link
Contributor

@rgrove
Copy link
Contributor

rgrove commented Apr 24, 2013

I don't have time to spin up all the A-grade browsers to run these tests, but these changes look sane on a read-through. I'd love it if @sdesai could take a look too, since he wrote many of the original Get tests.

@ekashida, I assume you've run these tests on all the browsers and the results match up with the pre-echoecho tests? More to the point, if you intentionally write failing tests using echoecho, do they fail as expected?

@ekashida
Copy link
Contributor Author

@rgrove I was testing with yogi serve and it was passing on all browsers, but I just tried a sanity check with yeti and they only fully pass on Chrome...

I'll look into this and also get a confirmation that tests using echoecho fail as expected.

@ekashida
Copy link
Contributor Author

It looks like I forgot to update my yeti dependencies after recently adding an echoecho feature. All A-grade browsers pass, except for Android 2.3 where not a single test passes. I'll need to debug this further tomorrow.

@ekashida
Copy link
Contributor Author

The problem with Android had to do with the fact that it didn't like the 404 that echoecho was returning via echo/status/404 for CSS files. I've changed the test to simply request a non-existant file and tests pass with flying colors in all A-grade browsers.

/cc @rgrove @sdesai

@ekashida
Copy link
Contributor Author

Also, echoecho tests that should fail, fail as expected!

@rgrove
Copy link
Contributor

rgrove commented Apr 26, 2013

Sweet! If @sdesai is happy with this, I am too.

@ericf
Copy link
Member

ericf commented Apr 30, 2013

Cool, this should make things less brittle, unless there's some brittleness in these tests which was explicit add (Satyen would know).

@@ -185,8 +260,9 @@ YUI.add('get-test', function (Y) {

'test: single script timeout callback': function() {
var test = this,
url = getUniqueEchoechoJs(null, { delay: 1 });
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid confusion, this can just be JS_A [ for someone reading the test, they have to go back and look up what "null" means ]. The important part is just the fixed/controlled delay. Also, along the same lines, to avoid future confusion, we can have the echoecho delay [ 1 ] be different from the Get.script() timeout delay

We were using the funky name below, to try and give the timeout a little bit more time to timeout, when we didn't have full control.

@sdesai
Copy link
Contributor

sdesai commented May 3, 2013

On the general 404 issue, Eugene and I discussed it offline, but I don't think we should try and work around it (as in the commit which changes echoecho/status/404 back to bogus.js).

The idea of porting Get tests over to echoecho was for complete reliability (even without echoecho, they're pretty reliable aside from one timeout test).

So, if echoecho is returning 404s which certain environments don't recognize as 404s then I think we should fix echoecho to provide 404 responses which work across the environments we're interested in testing, and wait to merge this in, until that is resolved.

@ekashida
Copy link
Contributor Author

ekashida commented May 8, 2013

After spending quite some time looking into this, we basically found that Android 2.3.4 is buggy when it comes down to link and script resources with the same URI.

Since 2.3.4 doesn't fire onload and onerror events for link tags, Get resorts to polling the DOM and comparing what's there vs. what is in document.styleSheets. If you append a link that has the same href as the src of a previously appended script node (echo/status/404 in our case), the link node fails to show up in document.styleSheets, and the polling mechanism which checks for link.href === document.styleSheets[].href never succeeds.

So we need to make echo/status/404 requests unique. I've cleaned this set of changes up along with implementing Satyen's feedback and will be opening another pull request to get them merged in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants