dsmiley commented on code in PR #4738:
URL: https://github.com/apache/solr/pull/4738#discussion_r3831018432
##########
solr/webapp/src/test/org/apache/solr/webapp/AdminUiTestBase.java:
##########
@@ -492,8 +523,8 @@ protected static void assertNoSevereConsoleErrors(String...
allowedSubstrings) {
/** Locates a Chrome/Chromium binary, or returns null if none can be found.
*/
@SuppressForbidden(reason = "Reading CHROME_BIN/PATH from the environment to
locate a browser")
protected static Path findChromeBinary() {
- String sysProp = EnvUtils.getProperty("tests.ui.chrome.binary");
- if (sysProp != null) {
+ String sysProp = EnvUtils.getProperty("tests.selenium.chrome.binary");
+ if (sysProp != null && !sysProp.isBlank()) {
Review Comment:
ugh; maybe there should be a getPropertyNotBlank
##########
solr/webapp/src/test/org/apache/solr/webapp/AdminUiTestBase.java:
##########
@@ -127,35 +127,50 @@ public abstract class AdminUiTestBase extends
SolrCloudTestCase {
protected static JettySolrRunner standaloneJetty;
/**
- * Serves a minimal stand-in for the generated js-client bundle ({@code
libs/solr/index.js}),
- * which only exists inside the built webapp, not in the source tree tests
serve from. The
- * AngularJS {@code CollectionsV2} service fails to instantiate without the
{@code solrApi}
- * global, taking the whole Collections screen down with it. Only the small
API surface the
- * AngularJS UI actually uses is stubbed.
+ * Serves the generated js-client bundle the AngularJS UI expects at {@code
libs/solr/index.js}:
+ * its {@code CollectionsV2} service fails to instantiate without the {@code
solrApi} global,
+ * taking the whole Collections screen down with it. The bundle is built by
{@code
+ * :solr:webapp:js-client} and its location handed to the test JVM in {@code
+ * tests.ui.jsclient.bundle}; it only exists inside the built webapp, not in
the source tree tests
+ * serve from.
*/
- public static class StubJsClientServlet extends HttpServlet {
+ public static class JsClientServlet extends HttpServlet {
+
+ private static volatile byte[] bundle;
+
@Override
protected void doGet(HttpServletRequest req, HttpServletResponse resp)
throws IOException {
resp.setContentType("text/javascript");
- resp.getOutputStream()
- .write(
- ("var solrApi = {\n"
- + " ApiClient: { instance: { basePath: '/api',
defaultHeaders: {} } },\n"
- + " CollectionsApi: function() {\n"
- + " this.reloadCollection = function(name, callback)
{\n"
- + " var xhr = new XMLHttpRequest();\n"
- + " xhr.open('POST', '/api/collections/' + name +
'/reload');\n"
- + " xhr.setRequestHeader('Content-Type',
'application/json');\n"
- + " xhr.onload = function() { callback(null, null,
{status: xhr.status}); };\n"
- + " xhr.onerror = function() { callback(new
Error('reload failed'), null, {status: xhr.status}); };\n"
- + " xhr.send('{}');\n"
- + " };\n"
- + " }\n"
- + "};\n")
- .getBytes(StandardCharsets.UTF_8));
+ resp.getOutputStream().write(bundle());
+ }
+
+ private static byte[] bundle() throws IOException {
+ byte[] cached = bundle;
+ if (cached == null) {
+ cached = Files.readAllBytes(jsClientBundlePath());
+ bundle = cached;
+ }
+ return cached;
Review Comment:
As a reviewer I key in on how people do IO streams and this hits a red flag
for me. You're reading an entire file into memory for no good reason. Just
transfer one stream to another; it's the same number of lines of code. No need
to cache.
--
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]
