Allow tests to access local version of web worker. #9475

kmh287 · 2017-05-22T21:43:09Z

kmh287 · 2017-05-22T22:32:36Z

src/web-worker/amp-worker.js

@@ -76,7 +76,7 @@ class AmpWorker {
    // Use RTV to make sure we fetch prod/canary/experiment correctly.
    const useRtvVersion = !getMode().localDev && !getMode().test;
    const url = calculateEntryPointScriptUrl(
-        loc, 'ww', getMode().localDev, useRtvVersion);
+        loc, 'ww', !useRtvVersion, useRtvVersion, getMode().minified);


Not sure about this change here. Without it, we're not pulling the web worker from local code. @choumx WDYT?

That's fine. Could use a better var name though.

const useLocalFile = getMode().localDev || getMode().test; const useRtvVersion = !useLocalFile;

kmh287 · 2017-05-23T13:19:37Z

src/service/extension-location.js

 * @return {string}
 */
 export function calculateEntryPointScriptUrl(
-    location, entryPoint, isLocalDev, opt_rtv) {
+    location, entryPoint, isLocalDev, opt_rtv, opt_minifed) {


Do you think opt_minified should be changed to opt_notMinified so that the non-max url is returned by defaukt?

Nah, inverted bools are harder to read.

dreamofabear

/cc @zhouyx @jridgewell

dreamofabear · 2017-05-23T14:59:30Z

src/service/extension-location.js

 * @return {string}
 */
 export function calculateEntryPointScriptUrl(
-    location, entryPoint, isLocalDev, opt_rtv) {
+    location, entryPoint, isLocalDev, opt_rtv, opt_minifed) {


Nah, inverted bools are harder to read.

dreamofabear · 2017-05-23T15:00:08Z

src/service/extension-location.js

-    return `${base}/rtv/${getMode().rtvVersion}/${entryPoint}.js`;
-  }
-  return `${base}/${entryPoint}.js`;
+  const version = opt_minifed ? '' : '.max';


dreamofabear · 2017-05-23T15:00:43Z

src/service/extension-location.js

-  return `${base}/${entryPoint}.js`;
+  const version = opt_minifed ? '' : '.max';
+  const rtv = opt_rtv ? `/rtv/${getMode().rtvVersion}` : '';
+  return `${base}${rtv}/${entryPoint}${version}.js`;


I think having the conditional is more readable here.

dreamofabear · 2017-05-23T15:07:23Z

src/web-worker/amp-worker.js

@@ -76,7 +76,7 @@ class AmpWorker {
    // Use RTV to make sure we fetch prod/canary/experiment correctly.
    const useRtvVersion = !getMode().localDev && !getMode().test;
    const url = calculateEntryPointScriptUrl(
-        loc, 'ww', getMode().localDev, useRtvVersion);
+        loc, 'ww', !useRtvVersion, useRtvVersion, getMode().minified);


That's fine. Could use a better var name though.

const useLocalFile = getMode().localDev || getMode().test; const useRtvVersion = !useLocalFile;

dreamofabear · 2017-05-23T15:08:31Z

src/service/extension-location.js

@@ -49,13 +49,13 @@ export function calculateExtensionScriptUrl(location, extensionId, isLocalDev) {
 * @param {string} entryPoint
 * @param {boolean=} isLocalDev
 * @param {boolean=} opt_rtv
+ * @param {boolean=} opt_minifed


opt_minified

dreamofabear · 2017-05-23T15:08:44Z

src/service/extension-location.js

 * @return {string}
 */
 export function calculateEntryPointScriptUrl(
-    location, entryPoint, isLocalDev, opt_rtv) {
+    location, entryPoint, isLocalDev, opt_rtv, opt_minifed) {


opt_minified

dreamofabear · 2017-05-23T15:09:45Z

test/functional/test-extension-location.js

-      expect(script).to.equal('http://localhost:8000/dist/sw.js');
+      },
+          'sw',
+          /* isLocalDev */ true);


This indentation looks weird.

I think so too, but this is what the linter wants. I think it's technically correct since the first argument is on the same line as the call, and the rest of the args are indented by four.

If these args can't fit on the same line, can you extract the object literal into a separate var instead? The linter ought to be a sanity check rather than the final authority on readability. 😃

jridgewell · 2017-05-23T17:21:04Z

src/service/extension-location.js

  const base = calculateScriptBaseUrl(location, isLocalDev);
-  if (opt_rtv) {
+  if (opt_rtv && opt_minified) {


const rtv = opt_rtv ? `/rtv/${getMode().rtvVersion}` : ''; const max = opt_minified ? '' : '.max'; return `${base}${rtv}/${entryPoint}${max}.js`

This is what I had before. @choumx @jridgewell which way do we want to go with?

Ours are almost 100% the same. I think that means it's the better option. 😉

Not a big deal, but I think having possibly empty strings in the format string makes it harder to verify that slashes, etc. are correct. Another option is to incrementally build the url string.

zhouyx · 2017-05-23T20:49:33Z

src/service/extension-location.js

 * @return {string}
 */
 export function calculateEntryPointScriptUrl(
-    location, entryPoint, isLocalDev, opt_rtv) {
+    location, entryPoint, isLocalDev, opt_rtv, opt_minified) {


@kmh287 Trying to understand why we need it back? I think we are trying to avoid this minified logic in PROD as much as we can.

To be able to run gulp test without --compiled. Currently we always serve the minified version which requires gulp dist to be run first.

We can remove this once unit tests no longer require gulp build.

Can't you set the test server to serve max files?

I thought we always serve unminified version when running gulp test without --compiled
https://github.com/ampproject/amphtml/blob/master/build-system/tasks/runtime-test.js#L181

Can't you set the test server to serve max files?

Hmm, we could do that. We'd need to dupe the logic in app.js into test-server.js.

I thought we always serve unminified version when running gulp test without --compiled

Only for the app.js server which currently doesn't run during testing.

Another issue is that location.host actually returns the Karma server on localhost, so that needs to change to the test server's port.

I thought https://github.com/ampproject/amphtml/blob/master/build-system/tasks/runtime-test.js#L196 did the work for us

Looks like that var refers to test-server.js. Perhaps we should consolidate though.

zhouyx · 2017-05-23T21:02:27Z

src/service/extension-location.js

    return `${base}/rtv/${getMode().rtvVersion}/${entryPoint}.js`;
+  } else if (opt_rtv) {
+    return `${base}/rtv/${getMode().rtvVersion}/${entryPoint}.max.js`;


can someone walk me through on how the opt-rtv work?

It fetches the current RTV file instead of the un-versioned file, e.g. cdn.ampproject.org/rtv/123/v0.js vs. cdn.ampproject.org/v0.js. We should never fetch max AND RTV though.

It's needed to support canary-ing of ww.js since it's fetched by AMP JS rather than from the AMP page.

I think better solution here is to always return ${base}/rtv/${getMode().rtvVersion}/${entryPoint}.js And let the local server choose what the right file to serve based on serving mode. Just like what we do with #calculateExtensionScriptUrl?

Yea, maybe we can make this change in test-server.js along with the other stuff above.

dreamofabear · 2017-05-23T22:48:01Z

@kmh287 A couple of good suggested changes, let's discuss it more tomorrow. 😄

kmh287

Thanks for the comments, everyone. I've updated the approach and hopefully addressed all previous comments. PTAL.

zhouyx · 2017-05-24T21:21:13Z

build-system/test-server.js

+/**
+ * Serve entry point script url
+ */
+app.get('/dist/ww.js', function(req, res, next) {


I wonder if this is needed.
We have https://github.com/ampproject/amphtml/blob/master/build-system/tasks/karma.conf.js#L188
where we import our gulp webserver as a Karma server middleware.

In app.js, around line 850, mode is not set to 'default' and so the min version is used.

When I comment out the added code in test-server.js, test-bind-impl.js pulls in the min version or outright fails if gulp dist hasn't been run since the last gulp clean. Witht his code added, the test pulls in the max version.

Yea, the middleware setting isn't working for some reason. Kevin, can you add a TODO/issue to investigate that?

we should eventually get rid of this 2nd server running in a different port. This probably can fix your problem: #9561

So close this PR in favor of #9561 ?

@lannka I've pulled in that PR and I'm still seeing errors while testing if I don't first run gulp dist. Without the changes to test-server.js, the test still requests the minified web worker.

In your test browser, can you try to load "http://localhost:9876/dist/ww.js"? It worked on our end.

I think he was saying we should eventually get rid of the second server, not that the referenced PR does it.

@lannka I get the min version only if I've run gulp dist since running gulp clean

@lannka and I resolved on chat. I needed to update karma.

kmh287 · 2017-05-26T22:24:06Z

@jridgewell @zhouyx @choumx @lannka PTAL

kmh287 requested a review from dreamofabear May 22, 2017 21:43

googlebot added the cla: yes label May 22, 2017

kmh287 commented May 22, 2017

View reviewed changes

kmh287 commented May 23, 2017

View reviewed changes

dreamofabear reviewed May 23, 2017

View reviewed changes

jridgewell requested changes May 23, 2017

View reviewed changes

zhouyx reviewed May 23, 2017

View reviewed changes

zhouyx suggested changes May 23, 2017

View reviewed changes

kmh287 commented May 24, 2017

View reviewed changes

kmh287 changed the title ~~Add support for fetching non-compiled URLs in extension-location#calculateEntryPointScriptUrl.~~ Allow tests to access max version of web worker. May 24, 2017

zhouyx reviewed May 24, 2017

View reviewed changes

kmh287 added 7 commits May 26, 2017 14:32

opt_minified

dec7b54

fix arg for localDev

f2403ab

pr comments

ce6b753

pr comments

59016f8

revert

65815d7

pr comments

4a51acf

revert changes to test-server and test-bind-impl

2e8b513

kmh287 force-pushed the bind_integration_test_fixes branch 2 times, most recently from d531fc1 to 387858e Compare May 26, 2017 19:39

cleanup

329da81

kmh287 force-pushed the bind_integration_test_fixes branch from 387858e to 329da81 Compare May 26, 2017 19:39

kmh287 changed the title ~~Allow tests to access max version of web worker.~~ Allow tests to access local version of web worker. May 26, 2017

jridgewell approved these changes May 26, 2017

View reviewed changes

zhouyx approved these changes May 26, 2017

View reviewed changes

dreamofabear merged commit 392bfad into ampproject:master May 26, 2017

ampprojectbot added the PR use: In Beta / Experimental label Jun 7, 2017

Allow tests to access local version of web worker. #9475

Allow tests to access local version of web worker. #9475

Conversation

kmh287 commented May 22, 2017

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

dreamofabear 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

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

dreamofabear May 23, 2017 • edited

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

dreamofabear commented May 23, 2017

kmh287 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

kmh287 May 26, 2017 • edited

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

kmh287 commented May 26, 2017

dreamofabear May 23, 2017 •

edited

kmh287 May 26, 2017 •

edited