SOLR-18347: Fix five Admin UI defects found by Selenium - #4818
SOLR-18347: Fix five Admin UI defects found by Selenium#4818NextbrickInc wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes an intermittent Admin UI navigation error where the core selector’s ng-change could invoke showCore() with a null/undefined model while the shared menu is still resolving, previously causing a TypeError when reading core.name. The change is localized to the AngularJS controller and tightens Selenium console-error checking so the regression is no longer silently ignored.
Changes:
- Add a null guard in
MainController.$scope.showCore(core)before dereferencingcore.nameand updating the route. - Remove the Selenium test exemption that filtered out the
"reading 'name'"/"showCore"console error so failures are surfaced.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| solr/webapp/web/js/angular/app.js | Guards showCore() against null/undefined core objects before routing. |
| solr/webapp/src/test/org/apache/solr/webapp/AdminUiTestBase.java | Removes the allowlist filter that previously ignored the showCore null-core console error. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Can you update PR title and description to match all five fixes? |
|
Thanks Jan — Yes I can . I updated the PR title and description to cover all five fixes. I also included the updated validation results: 22 Selenium tests across five browser-test classes, six ping-handler tests, and the changelog entry. |
# Conflicts: # solr/webapp/web/js/angular/controllers/collections.js # solr/webapp/web/js/angular/services.js
|
@janhoy I merged the latest Validation on the resolved tree:
The five GitHub Actions workflows are currently awaiting maintainer approval before they can run. The previous Crave run was cancelled by infrastructure while |
janhoy
left a comment
There was a problem hiding this comment.
A bit confusing to lump five fixes into one PR, but I'll not ask you to split them now. See comments
SOLR-18347-fix-report-secure.pdf |
"enabled" and "disabled" both mean a healthcheckFile is configured, so toggleHealthcheck() works and the widget should show its lit / unlit control. "not_configured" means there is no file at all, and enable/disable answer 503, so it needs the message branch that hides the toggle instead of offering a control that cannot work. Those three cases already behaved correctly. Any other value, though, fell into the else and rendered the unlit "enable ping" control, which is the one outcome an unknown status should not produce. Name the three known states and send everything else to the message branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks for going through it, @janhoy. On the packaging point — fair, and I would have split it if I could have. The five came out of one run: they are what the new Selenium suite reported the first time it ran without exemptions. Three of them are coupled through a single file, because I have answered all three inline comments. One of them turned into a change: 9b5545d makes the ping status handling in Validation for that commit: the full Admin UI Selenium suite with |
| * <p>The Analyze action remains disabled until creation of the mutable schema has completed, so a | ||
| * fast user cannot race the prep and analyze requests. | ||
| */ | ||
| @LuceneTestCase.AwaitsFix(bugUrl = "https://issues.apache.org/jira/browse/SOLR-18347") |
There was a problem hiding this comment.
BTW, if there is a lot of flakiness in this test, I am getting closer to migrating the Admin UI to using the generated V2 Javascript client, which I think is more reliable. And honestly, I think i might have made some commits that may even have broke the schema-designer.js now that I think of it (not sure), so it may be okay to not worry about this page... It's on my list to get to "done done'!
There was a problem hiding this comment.
Thanks @epugh — two things that might change the calculus.
It isn't flaky any more. The @AwaitsFix blamed "transient failures at automation speed"; the
real cause was deterministic. The designer addressed Luke by core name, and a core name only
resolves on the node hosting it — with configureCluster(2), that's the wrong node about half the
time. Addressing the collection fixes it: 53 tests, 18 suites, 0 failures.
Your hunch is right. SchemaDesignerResponse.core came in with SOLR-18152 (#4203), commented
"Active replica core name ... used for Luke API calls" — reasonable-looking, just not routable
from an arbitrary node. Nothing else in that migration looks affected; every Schema Designer
assertion passes on top of it.
So I'd keep the test rather than drop the page — it's what would have caught this. And the V2
client would issue the same request against the same core name, so this class of bug survives the
migration either way.
Status — I think this is ready to go in.
- @janhoy's three comments are answered; the one that needed a change is pushed as
9b5545ddb4. - Full Admin UI Selenium suite: 53 tests, 18 suites, 0 failures, 0 skipped.
tidyclean,
PingRequestHandlerTest6/6. - No open review threads and nothing outstanding on my side.
@janhoy — the only reservation you raised was the five-in-one packaging. If you're willing to take
it as-is, I'd appreciate an approval. If not, say so and I'll split it along
PingRequestHandler + Core Overview vs. the four UI-only fixes today.
@epugh — if the Schema Designer question above is settled for you, a second approval would let this
land.
|
Pls approve PR, merge code. Thank you so much . |
Thanks for the PR! Just so you know what to expect: Solr is a global volunteer project, so we usually leave a PR open for at least three days before merging, to give other committers a chance to review. Reminders are welcome once a PR has gone quiet — say after a week with no activity. Thanks for your patience in the meantime! |
https://issues.apache.org/jira/browse/SOLR-18347
Description
Fixes all five Admin UI defects listed in SOLR-18347:
showCore()with a null core and throw aTypeError.Solution
showCore()until a core object is present, and remove the Selenium console-error exemption for this failure.main: build and package the bundle into the WAR and serve it to Selenium tests sosolrApiis available when Collections and the other V2 screens initialize.status: "not_configured"for ping status when no healthcheck is configured, and handle every ping status explicitly in Core Overview so thatenabled/disabledkeep the working toggle whilenot_configured(and any unknown value) show a message instead of a control that would answer 503.Tests
Current revision —
9b5545ddb4./gradlew :solr:webapp:test -Ptests.selenium=trueagainst headless Chrome: 53 tests across 18 suites, 0 failures, 0 skipped. This is the wholesolr:webappmodule, not a targeted subset, and it includesAdminUiSchemaDesignerTestnow that its@AwaitsFixis gone../gradlew tidy: clean, no files modified.PingRequestHandlerTest: 6 tests passed.Note that the Selenium suites are opt-in behind
-Ptests.selenium=true; without the flag:solr:webapp:testreports "6 tests, 6 skipped", which looks like a pass but exercises nothing.Earlier revisions
./gradlew testwas still running; no test failure was logged.9b5545ddb4are awaiting Apache maintainer approval;labelerhas run and passed.Checklist
mainbranch../gradlew checkworkflow is awaiting maintainer approval. It passed on the previous revision.