Attempt to fix test__multiple_valid_image_files by kienstra · Pull Request #4034 · ampproject/amp-wp

kienstra · 2020-01-07T23:59:24Z

Summary

This attempts to fix test__multiple_valid_image_files. That test had failed intermittently.

Following the example of Weston's work on a FasterImage test, this hosts an image in a Gist instead of amp-wp.org
https://github.com/willwashburn/fasterimage/blob/42d125a15dc124520aff2157bbed9a4b2d4f310a/tests/FasterImageTest.php#L117

The actual file could be hosted in a Gist in Weston's account, if this approach is valid.

Checklist

My pull request is addressing an open issue (please create one otherwise).
My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

Following the example of Weston's work on a FasterImage test: https://github.com/willwashburn/fasterimage/blob/42d125a15dc124520aff2157bbed9a4b2d4f310a/tests/FasterImageTest.php#L117

westonruter · 2020-01-08T00:03:00Z

The intention behind hosting the file on amp-wp.org was to avoid it being tied to anyone's GitHub account. Since GitHub isn't a CDN it should seem that loading a file from a Gist would be less reliable than loading it from a production website, not more reliable.

kienstra · 2020-01-08T00:04:12Z

OK, good point.

kienstra · 2020-01-08T00:08:12Z

The external-http build that had failed before just passed. But it had also passed sometimes before. I'll run that build a few more times just to test this, and then revert the previous commit aadfaa1

It looks like you can't re-run a passed build, so I made a commit to trigger another build.

This reverts commit aadfaa1.

Using Weston's snippet, test if there are more failed images.

This reverts commit 2a513b3.

This reverts commit 12f51ea.

kienstra · 2020-01-08T01:12:08Z

Without aadfaa1, test__multiple_valid_image_files failed 1 of 2 times.

With aadfaa1, it passed 5 of 5 times.

Understanding that using a GitHub Gist isn't possible, I'll close this PR soon.

westonruter · 2020-01-08T01:33:47Z

Using a GitHub Gist is possible, but it doesn't seem correct. There must be some other reason for why this is happening. Perhaps the issue is there is some Pantheon protection in place against a bots trying to DDoS? I don't see why that would be the case and only for one file (and a static one at that), but it's a possibility.

kienstra · 2020-01-08T01:36:08Z

Yeah, I agree that it doesn't seem right to use a Gist.

It does seem strange that there's only one file failing in the test, I'm still not sure what the cause is.

westonruter · 2020-01-08T01:37:55Z

What happens if the order of items in the array is changed, so this SVG is fetched first?

kienstra · 2020-01-08T01:39:18Z

I'll try that.

Revert "Revert "Revert "Experiment with hosting a mock image on GitHub""" This reverts commit 734e92b.

As Weston suggested, in order to test if this still fails.

kienstra · 2020-01-08T01:52:53Z

Hm, the same .svg file failed, even with it moved to the beginning of the array.

westonruter · 2020-01-08T01:57:12Z

That somewhat rules out the timeout theory. So strange.

westonruter · 2020-01-08T02:12:00Z

Let's try clearing the Travis caches...

kienstra · 2020-01-08T02:25:40Z

Hm, it passed.

westonruter · 2020-01-08T02:26:52Z

If it passes 5 times in a row, then I think we found the problem.

kienstra · 2020-01-08T03:25:22Z

The external-http test passed, and I restarted it.

This reverts commit c5a2912.

Use mainly the same configuration as in FasterImage::handle().

I set the wrong order before, this needs to always run teh curl assertions.

Like before, as it looks like this prevents the failed external-http test. Still, I don't know why it failed before.

So that there are a few passing Travis builds to ensure this works.

kienstra · 2020-01-10T21:54:16Z

Unfortunately, I don't know why test__multiple_valid_image_files had failed.

4d7218c#diff-9d88cb48c8afba5f63546855275bb633R241 tested whether basic curl requests for https://amp-wp.org/wp-content/plugin-test-files/google.svg succeeded. And they did.

But FasterImage's curl requests use curl_multi_*.

When reproducing this locally by stepping through Xdebug breakpoints very slowly, there was never an exception when the curl request failed.

And it looks like FasterImage didn't throw exceptions in the failed Travis builds.

kienstra · 2020-01-10T22:10:50Z

Hi @westonruter,
What do you think about the approach here, understanding it's not ideal?

The test is reliably passing. But like you mentioned, it'd be much better to know why it failed.

Also, if you'd like, maybe the Gist could be from your GitHub account.

Thanks, and have a great weekend.

westonruter

Well. Let's just do it! 😄

* Experiment with hosting a mock image on GitHub Following the example of Weston's work on a FasterImage test: https://github.com/willwashburn/fasterimage/blob/42d125a15dc124520aff2157bbed9a4b2d4f310a/tests/FasterImageTest.php#L117 * Empty commit to trigger another Travis build * Another empty commit to trigger another Travis build * Revert "Experiment with hosting a mock image on GitHub" This reverts commit aadfaa1. * Yet another empty commit to trigger another Travis build * Test if setting a lower timeout causes more test failures Using Weston's snippet, test if there are more failed images. * Revert "Revert "Experiment with hosting a mock image on GitHub"" This reverts commit 2a513b3. * Revert "Test if setting a lower timeout causes more test failures" This reverts commit 12f51ea. * Another empty commit to ensure the test still passes * Revert entire diff of PR, now back in sync with develop Revert "Revert "Revert "Experiment with hosting a mock image on GitHub""" This reverts commit 734e92b. * Move the .svg file that failed to the beginning of the test images As Weston suggested, in order to test if this still fails. * Revert last commit, now no diff against the develop branch This reverts commit c5a2912. * Will revert: test if curl requests for images succeed Use mainly the same configuration as in FasterImage::handle(). * Change order of assertions so the plain curl test always runs I set the wrong order before, this needs to always run teh curl assertions. * Empty commit to trigger another Travis build * Another empty commit to trigger another Travis build * Will revert: pass the --verbose flag to phpunit in .travis.yml * Yet another empty commit to trigger another Travis build * Revert entire PR except for temporary changes to .travis.yml * Revert PR's diff from develop * Again change the amp-wp.org URL to a Gist URL Like before, as it looks like this prevents the failed external-http test. Still, I don't know why it failed before. * Empty commit to trigger Travis build So that there are a few passing Travis builds to ensure this works.

…izer-augmented-validate-response-data * 'develop' of github.com:ampproject/amp-wp: (41 commits) Apply workaround to fix test__multiple_valid_image_files (#4034) Update dependency @wordpress/compose to v3.10.0 (#4080) Update dependency @babel/cli to v7.8.3 (#4097) Update dependency @babel/plugin-transform-react-jsx to v7.8.3 (#4101) Update dependency @babel/plugin-proposal-class-properties to v7.8.3 (#4099) Update dependency @babel/polyfill to v7.8.3 (#4102) Update dependency @babel/plugin-proposal-object-rest-spread to… (#4100) Preserve styles applied via [style] attribute selectors Update dependency @wordpress/dom to v2.7.0 (#4083) Update dependency @wordpress/keycodes to v2.8.0 (#4090) Update dependency @wordpress/edit-post to v3.11.0 (#4086) Update dependency @wordpress/core-data to v2.10.0 (#4081) Update dependency @wordpress/server-side-render to v1.6.0 (#4093) Update dependency @wordpress/dom-ready to v2.6.0 (#4084) Update dependency @wordpress/components to v9 (#4094) Update dependency @wordpress/block-editor to v3.5.0 (#4078) Update dependency @wordpress/is-shallow-equal to v1.7.0 (#4089) Update dependency @wordpress/e2e-test-utils to v4.1.0 (#4085) Update dependency @wordpress/data to v4.12.0 (#4082) Update dependency @wordpress/rich-text to v3.10.0 (#4092) ...

* tag '1.4.3': (22 commits) Update readme and screenshots for Stories removal (#4259) Open story export instructions in a new window (#4258) Bump version to 1.4.3-RC1 Hide Stories options and add deprecation notice (#4219) Fix malformed conversion of relative action URLs for forms (#4250) Limit Stories experience to WP 5.3 & Gutenberg 7.1.0 (#4217) Prevent errors in admin bar filters from non-array arguments (#4207) Update @wordpress/e2e-test-utils dependency Revert update of mustache/mustache dependency Update composer.lock Update WP CLI to 2.4.0 For WordPress.tv embed, Use an oembed filter instead of block filter (#4164) Update readme to add FAQs section (#4159) Apply workaround to fix test__multiple_valid_image_files (#4034) Ignore Story editor tests (#4043) Update amp-video embed regex pattern to include other Vimeo URL formats (#4051) Update amp-instagram embed regex (#4053) Update wp-dev-lib package (#4029) Fix conversion of forms with relative action URLs (#4003) Improve release instructions (#3995) ...

Experiment with hosting a mock image on GitHub

aadfaa1

Following the example of Weston's work on a FasterImage test: https://github.com/willwashburn/fasterimage/blob/42d125a15dc124520aff2157bbed9a4b2d4f310a/tests/FasterImageTest.php#L117

googlebot added the cla: yes Signed the Google CLA label Jan 7, 2020

kienstra added 8 commits January 7, 2020 18:11

Empty commit to trigger another Travis build

40fc4b2

Another empty commit to trigger another Travis build

88172e0

Revert "Experiment with hosting a mock image on GitHub"

2a513b3

This reverts commit aadfaa1.

Yet another empty commit to trigger another Travis build

b9f2a6d

Test if setting a lower timeout causes more test failures

12f51ea

Using Weston's snippet, test if there are more failed images.

Revert "Revert "Experiment with hosting a mock image on GitHub""

734e92b

This reverts commit 2a513b3.

Revert "Test if setting a lower timeout causes more test failures"

705c052

This reverts commit 12f51ea.

Another empty commit to ensure the test still passes

03d8c6b

kienstra added 2 commits January 7, 2020 19:39

Revert entire diff of PR, now back in sync with develop

7abb719

Revert "Revert "Revert "Experiment with hosting a mock image on GitHub""" This reverts commit 734e92b.

Move the .svg file that failed to the beginning of the test images

c5a2912

As Weston suggested, in order to test if this still fails.

kienstra added 4 commits January 8, 2020 14:08

Revert last commit, now no diff against the develop branch

3ddc270

This reverts commit c5a2912.

Will revert: test if curl requests for images succeed

4d7218c

Use mainly the same configuration as in FasterImage::handle().

Change order of assertions so the plain curl test always runs

78629be

I set the wrong order before, this needs to always run teh curl assertions.

Empty commit to trigger another Travis build

d9cf40e

kienstra added 7 commits January 8, 2020 15:26

Another empty commit to trigger another Travis build

ad4b1b0

Will revert: pass the --verbose flag to phpunit in .travis.yml

06510a4

Yet another empty commit to trigger another Travis build

ccca96e

Revert entire PR except for temporary changes to .travis.yml

0fff0d9

Revert PR's diff from develop

4d85b79

Again change the amp-wp.org URL to a Gist URL

14631d2

Like before, as it looks like this prevents the failed external-http test. Still, I don't know why it failed before.

Empty commit to trigger Travis build

0bc1a50

So that there are a few passing Travis builds to ensure this works.

Merge branch 'develop' into try/image-test-fix

b4effc6

westonruter added this to the v1.4.3 milestone Jan 15, 2020

westonruter approved these changes Jan 15, 2020

View reviewed changes

westonruter merged commit a3ba286 into develop Jan 15, 2020

westonruter deleted the try/image-test-fix branch January 15, 2020 00:45

pierlon mentioned this pull request Jan 15, 2020

Fix flaky image test on Travis #4035

Closed

3 tasks

Conversation

kienstra commented Jan 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

westonruter commented Jan 8, 2020

Uh oh!

kienstra commented Jan 8, 2020

Uh oh!

kienstra commented Jan 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kienstra commented Jan 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

westonruter commented Jan 8, 2020

Uh oh!

kienstra commented Jan 8, 2020

Uh oh!

westonruter commented Jan 8, 2020

Uh oh!

kienstra commented Jan 8, 2020

Uh oh!

kienstra commented Jan 8, 2020

Uh oh!

westonruter commented Jan 8, 2020

Uh oh!

westonruter commented Jan 8, 2020

Uh oh!

kienstra commented Jan 8, 2020

Uh oh!

westonruter commented Jan 8, 2020

Uh oh!

kienstra commented Jan 8, 2020

Uh oh!

kienstra commented Jan 10, 2020

Uh oh!

kienstra commented Jan 10, 2020

Uh oh!

westonruter left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kienstra commented Jan 7, 2020 •

edited

Loading

kienstra commented Jan 8, 2020 •

edited

Loading

kienstra commented Jan 8, 2020 •

edited

Loading