Monitor external requests from tests on Travis #26756

powerivq · 2020-02-12T05:57:33Z

This PR implements #26754

Currently, it only prints on the log intercepted domains, and does not mark the test as failed. We can test running it for a while, see how stable it is, and enable that in the future.

amp-owners-bot · 2020-02-12T07:46:38Z

Hey @estherkim, these files were changed:

build-system/pr-check/e2e-tests.js
build-system/pr-check/local-tests.js
build-system/pr-check/single-pass-tests.js
build-system/pr-check/utils.js

Hey @rsimha, these files were changed:

build-system/pr-check/e2e-tests.js
build-system/pr-check/local-tests.js
build-system/pr-check/single-pass-tests.js
build-system/pr-check/utils.js

rsimha

Thanks for working on this PR. I think it will be valuable in reducing flakiness due to network requests.

I've requested a change to how gulp dns-monitor works. Happy to review once again.

rsimha · 2020-02-24T17:17:50Z

build-system/pr-check/utils.js

+  // if (!isTravisBuild()) {
+  //   return Promise.resolve(null);
+  // }


Is this necessary? What is the current behavior during local development?

Not strictly necessary because only pr-check/xxx.js references to it. I put it back as an extra layer of protection in case it is needed in special circumstances. It is supposed to be turned off during local dev, since

You might be doing other stuffs on your computer while running tests, and it can't distinguish them from those generated from tests

This needs to be ran as root, which is doable but needs some special handling.

Gotcha. In that case, we should just run this on Travis. Speaking of which, Travis VMs do background stuff too. For instance, if you're using this during a sauce labs run, there is a proxy on the Travis VM communicating with the Sauce VM.

Is it safe to say this should only be used for local unit, integration, and e2e tests?

/cc @estherkim for thoughts here.

I haven't turned it on for sauce lab runs. If we need to, we could whitelist them. (I've whitelisted Google Cloud-specific stuff travis uses and app engine stuff infra team uses) As long as they remain constant, it's not a problem.

rsimha · 2020-02-24T17:19:56Z

build-system/tasks/dns-monitor.js

+  /\.test$/, // .test TLD is safe to use for tests
+];
+
+const TCPDUMP_DNS_REGEX = /^(\d{2}:\d{2}:\d{2})\.\d{6} IP6? [^ ]+ > [^ ]+: \d+\+ (A|AAAA)\? ([^ ]+)\. \(\d+\)$/;


This is a complicated regex that will be hard to maintain. Can you split it into parts with inline comments for each part? Can you also add a comment above it explaining how it works at a high level?

Great idea. Will do.

rsimha · 2020-02-24T17:24:46Z

build-system/pr-check/utils.js

+/**
+ * Executes the command and check unpermitted DNS requests in the interim. In
+ * case if failure, it terminates.
+ * @param {string} cmd
+ * @param {string} fileName
+ */
+async function timedExecOrDieWithDnsMonitor(cmd, fileName = 'utils.js') {
+  return startDnsMonitor_().then(dnsMonitor => {
+    timedExecOrDie(cmd, fileName);
+    if (dnsMonitor) {
+      dnsMonitor.stdout.pipe(process.stdout);
+      dnsMonitor.stderr.pipe(process.stderr);
+      dnsMonitor.on('exit', code => {
+        if (code) {
+          process.exit(1);
+        }
+        resolver();
+      });
+      dnsMonitor.kill('SIGTERM');
+      let resolver;
+      return new Promise(resolverIn => {
+        resolver = resolverIn;
+      });
+    } else {
+      return Promise.resolve();
+    }
+  });
+}


I'd like to request a fundamental change to the way the gulp dns-monitor task works, which will make this function unnecessary.

Right now, the responsibility of running gulp dns-monitor in the background falls on the caller, and different callers will have to independently figure out how to start a background daemon.

Instead, I suggest you make gulp dns-monitor start as a daemon process on its own. That way, you can run something like this:

gulp dns-monitor --start gulp e2e gulp dns-monitor --stop

There is an example for this behavior in the gulp nailgun task:

amphtml/build-system/tasks/nailgun.js

Lines 186 to 187 in 1b24138

nailgunStart.description = 'Starts up a nailgun server for closure compiler';

nailgunStop.description = 'Stops an already running nailgun server';

I've used gulp nailgun as the blueprint when writing this! For this dns-monitor, I feel the biggest difference is that its return code (0 or 1) would dictate whether the Travis test should fail. We could do that in theory, but it would require a new way for doing inter-process communication.

As discussed offline, my main objection is that most of the code in timedExecOrDieWithDnsMonitor is specific to the dns monitor and doesn't belong in utils.js.

rsimha · 2020-02-24T17:25:16Z

build-system/pr-check/local-tests.js

@@ -28,6 +28,7 @@ const {
  startTimer,
  stopTimer,
  timedExecOrDie: timedExecOrDieBase,
+  timedExecOrDieWithDnsMonitor: timedExecOrDieWithDnsMonitorBase,


With my suggestion to change how gulp dns-monitor works, this file can be reverted.

rsimha · 2020-02-24T17:25:37Z

build-system/pr-check/e2e-tests.js

@@ -28,6 +28,7 @@ const {
  startTimer,
  stopTimer,
  timedExecOrDie: timedExecOrDieBase,
+  timedExecOrDieWithDnsMonitor: timedExecOrDieWithDnsMonitorBase,


With my suggestion to change how gulp dns-monitor works, this file can be reverted.

rsimha · 2020-02-24T17:25:44Z

build-system/pr-check/single-pass-tests.js

@@ -27,6 +27,7 @@ const {
  startTimer,
  stopTimer,
  timedExecOrDie: timedExecOrDieBase,
+  timedExecOrDieWithDnsMonitor: timedExecOrDieWithDnsMonitorBase,


With my suggestion to change how gulp dns-monitor works, this file can be reverted.

kristoferbaxter · 2020-02-25T03:29:25Z

Have you perused alternatives to doing this in JavaScript during test execution?

powerivq · 2020-02-25T03:57:44Z

@kristoferbaxter That is possible with unit/integration, but no good js options on e2e are available on browser (except for setting up a proxy).

kristoferbaxter · 2020-02-25T04:20:18Z

Implementing a proxy in JavaScript might be accomplished with an actual proxy as well.

Also, instead of a proxy have you considered a codemod + eslint rule?

powerivq · 2020-02-25T06:43:36Z

@kristoferbaxter a lot of times, the code that gives problem is the (AMP) html snippets here and there. I couldn't come up with a clean way to lint it with confidence, and, at the same time, be applicable to unit, integration and e2e. Ideas?

estherkim · 2020-02-27T14:56:16Z

I don't think a dns monitor makes sense with e2e tests, because asserting sent network requests is part of the framework. My gut feeling is that blocking external requests for the entire test suite is too prescriptive.

I like the javascript / lint idea; it's lightweight, can be overwritten per test if need be, and provides guidance to the developer in a quicker feedback loop, as opposed to the CI build. In fact, I'm not sure that the CI build is the correct place for this check.

@powerivq - can you post examples of unit and integration tests that send external requests?

powerivq · 2020-02-27T16:23:44Z

@estherkim #26651 this is an example.

It is okay for e2e tests to send out requests and to assert that, but the e2e test should not depend on the result of that external request. The point of this is to ensure that the external request is made against a known test domain. e.g amp-analytics needs to verify beacon URL is called. It can set the beacon URL to be https://example.test/view rather than https://www.example.com/view.

If absolutely necessary, a class of domains can still be whitelisted.

I would like to see linter work, because of the benefits you mentioned. It ended up either over-inclusive or under-inclusive. Also it would be difficult to cover e2e with linting rules, because URLs can appear anywhere in HTML.

mrjoro · 2020-06-11T18:43:35Z

Is this PR still active?

googlebot added the cla: yes label Feb 12, 2020

powerivq force-pushed the dns-mon branch from bcdae86 to 403fa0a Compare February 12, 2020 07:46

powerivq force-pushed the dns-mon branch 18 times, most recently from e3c06c9 to 2575952 Compare February 14, 2020 08:08

powerivq force-pushed the dns-mon branch 7 times, most recently from a1ba9c0 to 2dfa71c Compare February 20, 2020 21:51

WIP Test DNS monitor

028188d

powerivq force-pushed the dns-mon branch from 2dfa71c to 028188d Compare February 20, 2020 23:12

powerivq changed the title ~~WIP Test DNS monitor~~ Monitor external requests from tests on Travis Feb 20, 2020

powerivq requested review from rsimha and estherkim February 20, 2020 23:51

rsimha suggested changes Feb 24, 2020

View reviewed changes

rsimha mentioned this pull request Apr 30, 2020

Make AMP tests less flaky #14360

Closed

powerivq closed this Aug 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Monitor external requests from tests on Travis #26756

Monitor external requests from tests on Travis #26756

powerivq commented Feb 12, 2020 •

edited

Loading

amp-owners-bot bot commented Feb 12, 2020 •

edited

Loading

rsimha left a comment

rsimha Feb 24, 2020

powerivq Feb 24, 2020

rsimha Feb 24, 2020

powerivq Feb 24, 2020

rsimha Feb 24, 2020

powerivq Feb 24, 2020

rsimha Feb 24, 2020

powerivq Feb 24, 2020

rsimha Feb 26, 2020

rsimha Feb 24, 2020

rsimha Feb 24, 2020

rsimha Feb 24, 2020

kristoferbaxter commented Feb 25, 2020

powerivq commented Feb 25, 2020

kristoferbaxter commented Feb 25, 2020

powerivq commented Feb 25, 2020 •

edited

Loading

estherkim commented Feb 27, 2020

powerivq commented Feb 27, 2020

mrjoro commented Jun 11, 2020

	nailgunStart.description = 'Starts up a nailgun server for closure compiler';
	nailgunStop.description = 'Stops an already running nailgun server';

Monitor external requests from tests on Travis #26756

Monitor external requests from tests on Travis #26756

Conversation

powerivq commented Feb 12, 2020 • edited Loading

amp-owners-bot bot commented Feb 12, 2020 • edited Loading

rsimha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristoferbaxter commented Feb 25, 2020

powerivq commented Feb 25, 2020

kristoferbaxter commented Feb 25, 2020

powerivq commented Feb 25, 2020 • edited Loading

estherkim commented Feb 27, 2020

powerivq commented Feb 27, 2020

mrjoro commented Jun 11, 2020

powerivq commented Feb 12, 2020 •

edited

Loading

amp-owners-bot bot commented Feb 12, 2020 •

edited

Loading

powerivq commented Feb 25, 2020 •

edited

Loading