yuqi1129 commented on PR #12293:
URL: https://github.com/apache/gravitino/pull/12293#issuecomment-6462196666
The auth-readiness change looks reasonable, but I think we need to
distinguish it from the failure reported in #12281 before treating this PR as
its fix.
The existing Redux `fetchMetalakes` condition prevents a request before
authentication is ready, but does not automatically retry a skipped request.
Adding `isAuthReady` to these effects can therefore be useful: it triggers the
fetch when authentication becomes ready even if the pathname has not changed.
Could we add a regression test for that transition, including OAuth with a
delayed token?
I also ran the original `CatalogsPageTest` class against latest main
`eb11e9e1c7c435bccde4943e0fe1ce9c0f4753f2`, with Web V2 explicitly verified in
the server startup log, Docker dependencies, Chrome/ChromeDriver 145.0.7632.46,
and Java 17.0.8 on macOS arm64. `testSelectMetalake()` still failed, but this
run exposed a different failure from the original issue:
```text
CatalogsPageTest > testSelectMetalake() FAILED
AssertionFailedError: expected: <true> but was: <false>
at CatalogsPageTest.testSelectMetalake(CatalogsPageTest.java:602)
StaleElementReferenceException: stale element not found in the current frame
Command: isElementDisplayed
at CatalogsPage.verifyEmptyTableData(CatalogsPage.java:1080)
```
The helper first obtains a visible empty-state element with
`wait.until(visibilityOfElementLocated(...))`, then calls
`emptyElement.isDisplayed()` again. The failure occurred in that second call
after a DOM replacement. The catch converts it to `false`. This failed after
about 8 seconds, rather than the 60-second timeout in #12281.
Could you also look at making the empty-state check resilient to
re-rendering, by resolving and checking the element within the polling
condition, retrying stale references, and avoiding the redundant post-wait
element access? It should also verify that the target metalake has actually
been selected.
This run was on main, not on this PR's head, so it does not demonstrate
whether the auth change fixes the original timeout. There was also an earlier,
separate 5-second schema-list wait failure in `testClickCatalogLink()`. The
target test did execute; after it failed, its switch-back step was skipped, so
I stopped the remaining dependent cases rather than reporting a complete suite
result. No production fix was applied during the reproduction.
I would keep the auth-readiness improvement scoped and tested as such,
investigate the stale-element failure separately, and avoid claiming the
original timeout is resolved until it is validated.
--
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]
