search

ampproject / amphtml

The AMP web component framework.
https://amp.dev
Apache License 2.0
14.89k stars 3.89k forks source link

I2I Test to check for unnecessary network requests #26754

Open powerivq opened 4 years ago

powerivq commented 4 years ago

Tests might be flaky because they implicitly relied on making external-facing requests. To counter this, we could run a dns logger while tests are being ran to ensure they are not making external-facing requests, except those in a whitelist.

Setting travis config to redirect example.com is insufficient because we cannot cover all possible domains.

rsimha commented 4 years ago

I think something like this would help in alleviating flaky and long-running tests. Some questions:

/cc @ampproject/wg-infra for comments.

powerivq commented 4 years ago

How do we define "unnecessary"?

From what I see, the majority of usages are inadvertent, so the test owner should be able to decide. (e.g: have an amp-img tag in an ads that points to an external resource, click on an example.com link, etc) We can whitelist .test TLD since they never resolve so people have something that is not localhost for testing purpose.

Are there genuine use cases for tests to make external network requests?

I imagine very few cases in the actual test need to access Internet. The build system might need to talk to various things like amp-test-status-bot.appspot.com.

What if we forbid all external requests during CI?

That would be ideal. Visual-diff already got there that since you can do it easily in Puppeteer. In other places (unit, integration, etc), it is not as easy. It needs to make sure both Node and the browser are not making requests. DNS logging would be a one-size-fits-all solution. I imagine setting a proxy could be another solution we might want to consider.

mdmower commented 4 years ago

I am able to reliably reproduce several unit test timeouts when running the full git unit --headless suite, but not when running git unit --headless --files=.... I'm suspicious these failures are related to network requests, so I thought I'd post a sample log here in hopes that you could weigh-in. It looks like every one of these failures is an iframe'd video player:

Quick summary of components that frequently timeout in unit tests: amp-delight-player, amp-mowplayer, amp-nexxtv-player, amp-dailymotion. Let me know if you think this should be opened as a separate bug report. Thanks!

powerivq commented 4 years ago

@mdmower It would make sense to create individual issues for each potential flaky test. This issue proposes a mechanism to maintain a "good state" but extensions still need to be fixed in the first place.

rsimha commented 3 years ago

[Infra triage] A lot has changed since this issue was filed. Assigning to @rileyajones for visibility and to decide if / how / when to solve this.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.