-
Notifications
You must be signed in to change notification settings - Fork 9.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tests(smoke): remove external resource from dbw #15111
Conversation
@@ -468,7 +468,7 @@ <h2 id="toppy" style="background-image:url('');">Do better web tester page</h2> | |||
<script>window.fetch = () => new Promise(r => setTimeout(r, 90000));</script> | |||
|
|||
<!-- FAIL(is-on-https): requires a non-localhost http file --> | |||
<script src="http://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script> | |||
<script src="http://0.0.0.0:10503/dobetterweb/third_party/fake-jquery.js"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: why not localhost?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we explicitly ignore localhost-like urls in the audit. Except we aren't exhaustive so we miss this less common way to specifying localhost :P
This reverts commit 9aab415.
Copying my second opinion from chat proposing splitting jquery stack detection into a different smoke test:
|
There's nothing tricky about detecting jquery - it's as simple as it seems. Using the real library doesn't gain us anything imo. We have other smokes that test |
I think it's fine to land this PR as is. That being said, if it's simple enough to include the real jquery library then I think we should. One other option is to check in jquery into the repo as a fixture and use a relative path to include it (still dropping the http check). |
This will allow us to stop skipping this test in Smokerider.