[ZEPPELIN-6527] Fix flaky run-e2e-tests-in-zeppelin-web CI job (searchBlock Protractor timing)#5295
[ZEPPELIN-6527] Fix flaky run-e2e-tests-in-zeppelin-web CI job (searchBlock Protractor timing)#5295miinhho wants to merge 7 commits into
Conversation
Move the active classic zeppelin-web e2e coverage from Protractor to the existing Playwright harness. Add a classic Playwright project for the legacy /classic UI, port the active home and search block checks, and remove the obsolete Protractor/webdriver-manager entry points from zeppelin-web. Also update the frontend CI job to build the classic UI and run the migrated checks through zeppelin-web-angular's Playwright e2e flow.
voidmatcha
left a comment
voidmatcha
left a comment
There was a problem hiding this comment.
Nice work dropping Protractor and moving the classic UI checks to Playwright. The ported cases match the originals closely, and retiring the flaky run-e2e-tests-in-zeppelin-web job is a good cleanup.
A few things I'd tweak. I pushed them as separate commits on my fork so they're easy to cherry-pick or diff against.
-
Make the base URL overridable via PLAYWRIGHT_BASE_URL. Right now the config computes it into a local variable, so there's no way to point the run elsewhere. Collapsing it to one env var keeps local and CI consistent.
voidmatcha@7059e995d -
Run the classic suite through an e2e:classic script plus E2E_CLASSIC.
npm run e2e -- --project=classicleans on fragile classic-only detection, so an explicit env var and npm script make the :8080 run unambiguous.
voidmatcha@96ce23558 -
Document the classic conventions in e2e/AGENTS.md: that it's a frozen legacy surface, the locator exceptions the classic templates need, gating readiness on a classic signal instead of waitForZeppelinReady, the coverage exemption, how to run it, and the anonymous-only CI scoping.
voidmatcha@3b894aaea -
Skip the classic project in the auth CI leg. The original Protractor suite only ran anonymous, so scoping classic to the anonymous leg via E2E_MODE matches that.
voidmatcha@0b5b83309
Full diff: voidmatcha/zeppelin@8597767...review/pr-5295-fixes
One note: the red CI here isn't from your PR. It's the new-UI keyboard-shortcut spec being flaky, and I'm tracking that one separately: https://issues.apache.org/jira/browse/ZEPPELIN-6536
|
Thanks for the detailed review and for splitting the suggested fixes into separate commits. 😄 🙇♂️ I applied the changes for the Playwright base URL override, the explicit classic e2e script, the classic test conventions docs, and the auth-matrix classic skip. |
voidmatcha
left a comment
voidmatcha
left a comment
There was a problem hiding this comment.
Since collaborativeMode.spec.js was fully commented out and deleted rather than ported, could we leave a visible follow-up near the migrated classic specs?
For example, a skipped placeholder in zeppelin-web-angular/e2e/tests/classic/classic-collaborative-mode.spec.ts with a named TODO to port the old disabled collaborative editing scenario later.
The deleted file already pointed to https://issues.apache.org/jira/browse/ZEPPELIN-5674, so we could reference that there as well.
Not a blocker, but it would make the migration more traceable.
| const countSubstringOccurrence = (text: string, substring: string): number => | ||
| (text.match(new RegExp(substring, 'g')) || []).length; |
There was a problem hiding this comment.
countSubstringOccurrence() uses new RegExp(substring, 'g'). Current terms are plain (t, te), but literal counting would be safer for future regex metacharacter cases.
| const countSubstringOccurrence = (text: string, substring: string): number => | |
| (text.match(new RegExp(substring, 'g')) || []).length; | |
| const countSubstringOccurrence = (text: string, substring: string): number => | |
| substring ? text.split(substring).length - 1 : 0; |
|
Could you rebase this onto master? |
|
I resolved the conflict via GitHub, which updated the branch with master and made it mergeable. I realized afterward that this produced a merge commit rather than a rebase. Happy to redo it as a local rebase and force-push if you prefer the linear history. |
2 participants
What is this PR for?
This PR removes the flaky classic
zeppelin-webProtractor e2e job by moving its remaining active coverage to the existing Playwright e2e workflow.The old
run-e2e-tests-in-zeppelin-webjob depended on Protractor, webdriver-manager, and a pinned ChromeDriver 2.35, and had been failing intermittently in the classic search/replace tests due to AngularJS/Ace synchronization timing.Since that stack is end-of-life and cannot be reasonably modernized in place, this PR ports the 10 active checks from
home.spec.jsandsearchBlock.spec.jsto Playwright, pointed at the classic/classicUI.The migrated tests keep the existing classic UI coverage for the home page, interpreter permission save flow, search shortcut, match counters, replace, and replace-all behavior. After that coverage is available in Playwright, the obsolete Protractor config, scripts, dependencies, and CI job are removed.
What type of PR is it?
Improvement
What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-6527
How should this be tested?
For the Maven/CI path:
./mvnw clean install -DskipTests -am -pl python,zeppelin-jupyter-interpreter,zeppelin-web,zeppelin-web-angular -Pweb-classic xvfb-run --auto-servernum --server-args="-screen 0 1024x768x24" ./mvnw verify -pl zeppelin-web-angular -Pweb-e2eQuestions:
zeppelin-web/README.mdto reflect migrated classic web e2e test