I2D: mode.isLocalDev

[rcebulko]

The code looks like this:

export function isLocalDev(opt_win) {
  if (isProd()) {
    return false;
  }

  return !!self.AMP_CONFIG?.localDev || isTest(opt_win);
}

It's basically always used like mode.isLocalDev(win) || mode.isTest(win)
I can't think of any environment where isProd() is false that isLocalDev should also be false. This is causing challenges with testing/running Bento components, since AMP_CONFIG isn't available.

Can we drop this entirely and replace all calls with !mode.isProd()?

[rcebulko]

/cc @jridgewell @samouri @erwinmombay @ampproject/wg-components

[samouri]

I can't think of any either. I have the same question about isTest. Are both just the same thing as !isProd?
We can implement this in two steps:

1. Modify the values within core/mode for the two to be the same.
2. Change all of localDev references to test and remove localDev

[samouri]

Another way of thinking about it is that there are three possible states we care to support:

1. Prod
2. Test
3. Test spoofing that it is prod (i.e. to validate cdn urls)

[samouri]

status update

After merging #35823, three things blew up. Ideally we (I) would have foreseen the potential breakages before merging, but here we are. Previously to that PR, isTest would be false and isLocalDev true when running the amp lazy server.

There are some (not many) checks for if (isTest()) {} where it is intentionally only taking the path during test and not also in local server. I currently believe all of these are smells that we should address:

1. amp story shadow dom

In prod (and local dev) amp-stories utilizes shadow dom for certain components. In test we render the same components in light dom instead. We do this because Percy cannot render shadow dom.

Since all test of isTest now renders these elements in light dom, unit tests have been written using element.querySelect to select for items that actually render in shadow dom in prod. This may be hiding subtle bugs and is IMO a bad pattern to continue following. If one day Percy is able to render shadow doms (e.g. via DSD) and we can remove the hack, we'll be held back by our test suite.

amphtml/extensions/amp-story/1.0/utils.js

Line 92 in a41366d

const containerToUse = getMode().test

Two potential ways to move forward here would be:

1. Create a method to detect if we are running in Percy (isPercy) and only branch to light dom rendering in that case.
2. Figure out how to render shadow doms in percy (Daniel has hinted that we can run dynamic js with full dom access during tests)

2. Virtual history vs. Natural History

During test, HistoryBindingVirtual was always used. During local dev we need HistoryBindingNatural.
We only actually needed to force VirtualHistoryBinding in viewer-like cases within the test (i.e. when within an iframe). Any time BindingNatural breaks within test it is due to either isolation issues or a genuinely faulty test.

Fixing in #35889

3. amp-story sticky header

Stopped sticking. Following up in #34504

[rcebulko]

Instead of amp-story logic branching on isTest, can we stub out the Shadow DOM APIs during testing so they actually write to the Light DOM?

[kristoferbaxter]

We do this because Percy cannot render shadow dom.

Is there a solution to this problem we can address?

[samouri]

We do this because Percy cannot render shadow dom.

Is there a solution to this problem we can address?

This would be the ideal situation and is the second solution I pointed out. @danielrozenberg was hinting that there exists "dynamic" percy tests which can execute JS instead of needing inert dom. A second possibility would be to use Declarative Shadow DOM.

[danielrozenberg]

This would be the ideal situation and is the second solution I pointed out. @danielrozenberg was hinting that there exists "dynamic" percy tests which can execute JS instead of needing inert dom. A second possibility would be to use Declarative Shadow DOM.

After sleeping on it, I realized that the dynamic tests feature is not the best approach here. I thought of a better solution, but first here's the TL;DR of how we execute the tests (skipping details that aren't relevant to this problem):

1. The test runner loads an AMPHTML page as defined in the visual_tests file
2. Waits for the page to load
3. "Freezes" parts of the page that might not be reflected in the HTML, such as any form data that was "typed" during the test or generated images in <canvas> elements, and add those to the HTML tree
4. And finally, takes an .outerHTML snapshot of its root <html> element, and uploads that to Percy

My proposed solution is to add another "freezing" task to step 3 that converts any shadow DOMs in the page to Declarative Shadow DOM. Is this something we can do? I'm not very familiar with DSD (I did check and DSD is only supported starting Chrome 90. Our Percy project was set to use Chrome 87 but I just changed the settings to update it and sent out PR #35942 to upgrade the local browser we use to match with the new version on their service)

For more full context on how visual diff test are written and executed, see the testing docs

[samouri]

@danielrozenberg: thank you for the explanation of how our visdiff tests are stitched together!

instead of an .outerHTML-based snapshot, can we use getInnerHTML({includeShadowRoots: true}). Unfortunately I don't believe a getOuterHTML function for serializing shadow roots exists, so we'll need to manually stitch it together.

Alternatively the "freezing" technique is likely to be slightly less brittle I believe the DSD serialization API is still experimental (getInnerHTML)

[danielrozenberg]

instead of an .outerHTML-based snapshot, can we use getInnerHTML({includeShadowRoots: true})

In reality we just pass a Puppeteer.Page object to the Percy library, and they extract the data. I used .outerHTML as a proxy for what it does.

If you want to follow the code: we call percySnapshot from the @percy/puppeteer package, which in turn calls serialize from the @percy/dom package (the serialize function is an alias of serializeDOM). The final returned value is doctype(dom) + doc.outerHTML;

Alternatively the "freezing" technique is likely to be slightly less brittle I believe the DSD serialization API is still experimental (getInnerHTML)

I think this is the only realistic option, or you can propose a solution to Percy directly (I'm sure it'll be appreciated!)

[samouri]

Judging by this GH Issue it doesn't look like Percy will support DSD/SD Serialization until there is more cross-browser support. On the percy-side, when they load the serialized HTML before taking a screenshot, do you know if they execute the page's JS?

If yes, then we could pre-process the document with a new stage that serializes the SD and inserts a DSD polyfill (only a few lines of code)

[danielrozenberg]

Judging by this GH Issue it doesn't look like Percy will support DSD/SD Serialization until there is more cross-browser support.

Oooh good find, didn't think to look at whether there's already an issue for this :)
We can still ask if they'd be willing to accept a PR (it could be a behind-a-flag feature for now) even if they themselves don't want to prioritize this

On the percy-side, when they load the serialized HTML before taking a screenshot, do you know if they execute the page's JS?

Yes, when you create a snapshot you pass an options object, one of the fields is enableJavaScript?: boolean which defaults to false.

If yes, then we could pre-process the document with a new stage that serializes the SD and inserts a DSD polyfill (only a few lines of code)

I don't think we need a DSD polyfill since we now run our Percy tests on Chrome 91 which has built-in, default-on support for DSD. Pre-processing to serialize the SD would probably be enough!

[samouri]

@danielrozenberg: I've implemented the serialization changes in #35917.

In order to complete the change though, there are a few other AIs:

1. Fix all the unit tests that were querying lightdom to query shadow. as long as we make a simple helper it should be a simple swap.
2. Modify all the "wait"s within the visdiff query selectors to wait for elements in shadow instead of light

[stale]

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.