viirya commented on code in PR #59595:
URL: https://github.com/apache/spark/pull/59595#discussion_r4318617574


##########
core/src/test/scala/org/apache/spark/deploy/master/MasterWorkerUISuite.scala:
##########
@@ -133,6 +133,11 @@ class MasterWorkerUISuite extends MasterSuiteBase {
           
.tryWithResource(Source.fromURL(s"$masterUrl/"))(_.getLines().mkString("\n"))
         html should include ("Spark Master at spark://")
         verifyStaticResourcesServedByProxy(html, reverseProxyUrl)
+        Seq("environment/", "logPage/?self&logType=out").foreach { path =>
+          val pageHtml = Utils
+            
.tryWithResource(Source.fromURL(s"$masterUrl/$path"))(_.getLines().mkString("\n"))
+          verifyStaticResourcesServedByProxy(pageHtml, reverseProxyUrl)

Review Comment:
   nit: when this fails, the message is just `"/" did not start with substring 
"http://proxyhost:8080/path/to/spark"`. The Environment and Log pages both have 
a `/` Back link, so it doesn't say which page failed. Maybe wrap the check in 
`withClue(s"$path: ") { ... }`?



##########
core/src/test/scala/org/apache/spark/deploy/master/MasterSuiteBase.scala:
##########
@@ -179,6 +182,8 @@ trait MasterSuiteBase extends SparkFunSuite
     html should include (s"""href="$proxyUrl/static""")
     html should not include ("""src="/static""")
     html should include (s"""src="$proxyUrl/static""")
+    // other site-relative links, e.g. "Back to Master", must not bypass the 
proxy either
+    (SITE_RELATIVE_LINK_RE findAllMatchIn html).foreach(_.group(1) should 
startWith (proxyUrl))

Review Comment:
   nit: every match starts with `/`, and all callers pass an absolute 
`proxyUrl` (`http://...`), so this effectively asserts that there are no 
site-relative links at all. Writing it that way states the intent directly and 
lists every offender in one failure, instead of stopping at the first one:
   
   ```scala
   SITE_RELATIVE_LINK_RE.findAllMatchIn(html).map(_.group(1)).toSeq shouldBe 
empty
   ```
   
   This also subsumes the `html should not include ("""href="/static"""")` 
check above.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to