Clean up previews_lite_page_browsertest.cc |
||
Issue description
Throughout the implementation of Lite Page Server previews, the browsertest has gotten increasingly dirty looking.
* Don't override the host name of the previews server. Use `host_resolver()->AddRule("*", "127.0.0.1")` instead.
* Parameterize the setup so that we don't need all the subclasses
* Revisit Mac and Windows disabling, check that it is still needed
* Make a single macro for disabling tests on certain platforms
,
Oct 29
The above CL takes care of
* Revisit Mac and Windows disabling, check that it is still needed
* Make a single macro for disabling tests on certain platforms
This:
* Parameterize the setup so that we don't need all the subclasses
isn't easily supported in gtest and making it work is a lot grosser than just having a few separate test fixtures so I'll leave it alone
This:
* Don't override the host name of the previews server.
could be done, but I don't think it really helps anything. In the end I will still need to override the previews server URL to point it at the correct EmbeddedTestServer port. Furthermore, adding a rule like
`cmd->AppendSwitchASCII("host-rules", "MAP * 127.0.0.1");`
concerns me a little bit since it'd be harder to catch weirdness like an erroneous navigation to a public domain (cnn.com, for example). The above rule means that cnn.com would resolve to localhost where anything could be happening on ports 80,443 but leaving the host rules as is means that the try bots would fail since they don't have network access when the erroneous navigation to cnn.com comes along.
Similar concerns for mapping just the previews server to localhost where I don't really want it to get to localhost if the test was setup incorrectly.
Marking as fixed.
|
||
►
Sign in to add a comment |
||
Comment 1 by bugdroid1@chromium.org
, Oct 29