Reconcile the readonlyrest_kbn in-repo Cypress copy into this suite

[sscarduzio]

Reconciliation before the in-repo Cypress copy in sscarduzio/readonlyrest_kbn (automatic-tests/cypress/) is deleted. Every shared spec and every shared support/ file was diffed against this repo, so nothing the KBN copy gained is lost when that copy goes away.

Result of the sweep

26 specs are shared. 14 of them differ, plus 16 shared support/ files, plugins/index.ts and all 15 fixtures. In almost every case this repo is the newer side: the KBN copy predates the cypress-recurse rewrites, the report-store pruning, the Loader/RorMenu hardening and the uncaught:exception narrowing that landed here.

Three things were only in the KBN copy. One line of them lands here.

Ported

ror-config.cy.ts — the skip names what lifts it. The KBN copy carries the reason on describe.skip inside an eslint-disable directive for a jest rule this repo does not use. Here it is one line: the suite is unskipped when #116 merges.

Looked at, not ported

Kibana-content-overwrite.cy.ts — the second Rules -> Connectors pair. The KBN copy walks Rules -> Connectors a second time with no homepage in between, meant as the RORDEV-2185 regression (readonlyrest_kbn#1028). It is not one: the released-plugins legs run -ror-latest, which is the same digest as -ror-1.71.0 (2026-09-04, before #1028), and the spec passes there on 9.5.3, 9.4.6 and 8.19.21. StackManagement.openConnectorsPage() goes through the global nav's Stack Management entry, so the route passes the management landing page, like the pair above it. A real regression test is Rules page -> KibanaNavigation.openSubPage('Connectors') through the management side nav; it cannot go green on a pull request here until a release with #1028 ships, because pull requests run only the released-plugins legs.

Spaces.cy.ts — the skip rationale. The existing FIXME line already says it.

Not ported, and why

• Discover.cy.ts credentials. KBN uses user2:dev, this repo uses user4:dev. Both name the same "defined indices" block (indices: [kibana_sample_data_*], .default_index); only the block name differs between the two ROR configs. Environment-specific.
• Tenancy.cy.ts, Reporting-index.cy.ts, Sanity-check.cy.ts, Loader.ts, Tenancy.ts, RoAndRoStrictKibanaAccessAssertions.ts imports. KBN reaches into the plugin's shared/constants/queryStringKeys; this repo has support/types.ts. Structural, and correct on each side.
• commands.ts uncaught-exception list, EsApiAdvancedClient legacy-index guard, No-non-ok-responses-after-login intercept scope. This repo changed all three deliberately and documented each one. See the open question below for the one part of that worth a second look.
• Fixtures and plugins/index.ts paths. ECK blocks, cert paths and hide_apps entries belong to each repo's own environment.

Needs a decision

One judgment call was left out rather than guessed.

commands.ts handles the lazy-chunk load failure differently on the two sides. The KBN copy suppresses it globally, gated to 8.x, in two wordings:

(isKibana8x && err.message.includes('ChunkLoadError')) ||
(isKibana8x && err.message.includes('Loading chunk')) ||

This repo suppresses one wording globally and ungated, and keeps the other two 8.x entries at spec scope in User-settings.cy.ts, where the theme reload produces them:

err.message.includes('Loading chunk') || // kibana 9.3.2 fails to fetch lazily loaded plugin chunks; affects every spec, so it stays global

Webpack's ChunkLoadError normally carries Loading chunk N failed in its message, so the global entry here probably already catches it. If it does not on some version, a spec other than User-settings that hits the failure fails where the KBN copy would not. Question for the reviewer: should ChunkLoadError sit beside Loading chunk in the global list, or is the narrower scope intended? Nothing was changed either way.

After this merges

Deleting the shared specs from automatic-tests/ loses no coverage, with one exception that is already known and stays where it is: Kibana-config.cy.ts. It really runs in the KBN IT job (10 tests). Here it is describe.skip on every environment, because changeKibanaConfig rewrites kibana.yml and restarts Kibana - the docker env runs two Kibana replicas that then disagree about the active config, and the ECK envs mount kibana.yml read-only from a ConfigMap, so the rewrite answers EROFS. ror-config.cy.ts is skipped on both sides, so it carries no coverage either way.

The version matrix is not a concern: the KBN IT job resolves one ELK version per major (9.5.3 / 8.19.21 / 7.17.x), and this repo's matrix is a superset (9.5.3, 9.4.6, 8.19.21, 7.17.29) across docker, eck-2.16.1 and eck-3.5.0. Activation-keys.cy.ts is skipped on docker here for the two-replica reason, and still runs on both ECK environments.

Observability.cy.ts should stay in this repo only. It needs an APM server and the environments/common/images/node-apm-app container, which readonlyrest_kbn/local-testing does not have, and the KBN ROR config hides the app outright (hide_apps: ["Enterprise Search|Overview", "Observability"]).

Verification

yarn tsCheck and yarn lint both pass in e2e-tests/ at zero errors. The 18 lint warnings are pre-existing and in files this PR does not touch.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

Next included review available in 52 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: a02d93b8-ba30-4d89-8915-6fa74d9dcd96

📥 Commits

Reviewing files that changed from the base of the PR and between 6b0814e and 587df40.

📒 Files selected for processing (1)

• e2e-tests/cypress/e2e/ror-config.cy.ts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[sscarduzio]

/review

[10hexdev]

The review is complete. My correct review posted with one inline finding anchored at ror-config.cy.ts:7, plus a body answering the PR's ChunkLoadError decision question.

Review posted: #133 (review)

[10hexdev]

review_test

[10hexdev]

inline_test

[10hexdev]

Verdict

Approve. The change does what it claims: it removes repeated and dead work without deleting a test from the matrix, weakening an assertion, or dropping an ELK version.

Verified

• ci/e2e-matrix.sh emits 8 legs for the default pairing and 8 for --flip; the two together cover all 12 (version, environment) cells, and the arithmetic matches the PR's table for both pairings (run: ./ci/e2e-matrix.sh matrix and matrix --flip).
• CI on the head commit 6fb50d2 is green: 8 suite legs passed, bootstrap and dev correctly skipped.
• runTests() still runs three times; the two tests left in it read both callbacks, so moving the other three out drops no assertion.
• The on('after:spec') merge is correct: one handler now does both jobs, and the earlier two-registration bug is gone.
• set -euo pipefail with [ schedule ] && FLIP=--flip does not exit when the condition is false (verified in bash).
• The deleted specs and fixtures leave no dangling references except the one method noted below.

Index

• nit: all-e2e-tests.yml:245 — the versions output of prepare-dev-images is now dead.
• nit: RorApiInternalKbnClient.ts:41 — changeKibanaConfig is now dead (outside the diff, so in the body below).

One number to fix

The PR body says "13 test bodies instead of 17" in Tenancy.cy.ts. The file yields 11: two runTests tests x three passes (6) plus five top-level tests (5). The share-link test also moved out of runTests (commit ee7e8f2), which the "13" does not count. Update the description to 11 — the saving is larger than claimed, not smaller.

---

Findings outside the PR's changed files

.github/workflows/all-e2e-tests.yml:245 — .github/workflows/all-e2e-tests.yml is not among the PR's changed files

nit: prepare-dev-images still writes a versions output nobody reads.

dev-e2e-tests no longer consumes it — its matrix is now ${{ fromJSON(needs.setup.outputs.e2e) }} (line 263), so the only consumer of needs.prepare-dev-images.outputs.versions is gone. Only run_tag (line 265) is still read.

The outputs.versions declaration at all-e2e-tests.yml:218 and this echo are dead. Delete both, or keep the output and route dev-e2e-tests back through it.

---

e2e-tests/cypress/support/helpers/RorApiInternalKbnClient.ts:41 — e2e-tests/cypress/support/helpers/RorApiInternalKbnClient.ts is not among the PR's changed files

nit: changeKibanaConfig is now dead code.

Its two callers (Kibana-config.cy.ts, ror-config.cy.ts) and all six fixtures it could load were deleted in this PR. No remaining code calls it — only a docstring in KbnApiAdvancedClient.ts:43 mentions it. rorApiInternalKbnClient itself is still used by Impersonate.cy.ts for getLicense/deactivateTestSettings, so only this method is dead.

Delete the method and its now-unreachable fixture path, or note where the next config test will re-add it.

[10hexdev]

Verdict

COMMENT — one comment-accuracy finding on the single changed line, plus an answer to the ChunkLoadError question the PR body asks. No blocker.

Verified sound

• The substantive change was correctly reverted. At head 587df40, Kibana-content-overwrite.cy.ts and Spaces.cy.ts are byte-identical to origin/master. The second Rules→Connectors pair was removed after Dzuming showed it cannot pass against the released -ror-latest image — the same digest as -ror-1.71.0, pushed 2026-09-04, before readonlyrest_kbn#1028 merged 2026-09-08. The updated PR body states this correctly.
• The roStrictSettings.yaml change was folded in via the master merge (6b0814e) and now matches origin/master exactly.
• All four review threads are resolved.

Findings index

1. ror-config.cy.ts:7 — the skip comment names the wrong outcome for #116 (decide before merge).

Answer to the "Needs a decision" question

ChunkLoadError is the error name, not part of the message. webpack's runtime sets error.message = 'Loading chunk ' + chunkId + ' failed.\n(…)' and error.name = 'ChunkLoadError' (webpack/lib/web/JsonpChunkLoadingRuntimeModule.js). Cypress's uncaught:exception handler tests err.message, so err.message.includes('ChunkLoadError') can never match a real webpack chunk-load failure; the existing global err.message.includes('Loading chunk') entry at commands.ts:249 already catches it.

So do not add ChunkLoadError to the global list — it would match nothing. The two ChunkLoadError message checks (the KBN copy's automatic-tests/cypress/support/commands.ts:236 and this repo's User-settings.cy.ts:26) are no-ops and can be dropped, with the comment noting the token lives in name, not message. The narrower spec scope is neither needed nor intended.

[10hexdev]

ror-config.cy.ts is deleted, not unskipped, by #116.

$ gh api repos/beshu-tech/readonlyrest-e2e-tests/pulls/116/files
  e2e-tests/cypress/e2e/ror-config.cy.ts | removed

The replacement spec #116 adds, Readonlyrest-settings.cy.ts, has seven it() blocks (disable multitenancy, index based session, custom CSS, custom JS, custom middleware, whitelisted Urls, tenancy) — none is should save ReadonlyREST Kibana config to the index, and none asserts Login.verifyLoginPageTitle('Loaded from index!') after RorMenu.pressLogoutButton().

So "It will be unskipped when … #116 is merged" names the wrong outcome: the file is removed, and its one save-to-index test has no replacement in #116.

Fix the comment to state what #116 does (deletes the file and migrates the settings coverage to Readonlyrest-settings.cy.ts without the save→logout→login-title assertion), or confirm with the #116 author that an equivalent test lands there before this merges.

[Dzuming]

LGTM