Use getMode().test to indicate testing environment during test #3699
Conversation
| @@ -196,8 +196,8 @@ export function getBootstrapBaseUrl(parentWindow, opt_strictForUnitTest) { | |||
| * @return {string} | |||
| */ | |||
| function getDefaultBootstrapBaseUrl(parentWindow) { | |||
| if (getMode().localDev || parentWindow.AMP_TEST) { | |||
| const prefix = parentWindow.AMP_TEST ? '/base' : ''; | |||
| if (getMode().localDev || getMode().test) { | |||
There was a problem hiding this comment.
@erwinmombay This should still be DCEable with our custom pass, right?
There was a problem hiding this comment.
@cramforce yes, will continue working on that. will need to expose it at module level instead of a value from getMode(). was struggling with getting tests to pass with my DCE changes but should be close.
|
LGTM |
| @@ -71,7 +71,7 @@ export function createFixtureIframe(fixture, initialIframeHeight, opt_beforeLoad | |||
| // on that window. | |||
| window.beforeLoad = function(win) { | |||
| // Flag as being a test window. | |||
| win.AMP_TEST = true; | |||
| win.AMP_TEST_IFRAME = true; | |||
There was a problem hiding this comment.
You still need to set AMP_TEST here.
There was a problem hiding this comment.
I assume AMP_TEST belongs to the global window, not the testing iframe. I set it here
There was a problem hiding this comment.
Maybe these names should be changed, but I think the semantics are simple:
AMP_TEST: behave specific to a unit test. This should e.g. trigger localhost-y URLsAMP_TEST_IFRAME: This is a per-test iframe. Behave accordingly.
With that you either always want to set AMP_TEST when AMP_TEST_IFRAME is set, or make getMode look at both variables.
There was a problem hiding this comment.
Yeah that's the semantics I mean.
The getMode method currently is reading cached mode variable once the mode is set (see this) Shall I update the mode variable every time I set window.AMP_TEST? If that's the case, we can just set mode.test to true for every unit test and we don't even need to set window.AMP_TEST.
I don't understand the usage of AMP_TEST_IFRAME. Why do we need such a flag for each testing iframe?
There was a problem hiding this comment.
I think we are speaking entirely passed easy other. Lets chat in person.
There was a problem hiding this comment.
Set window.AMP_TEST to true.
|
@cramforce PTAL |
|
LGTM |
No description provided.