[Observability Onboarding] Add Wired Streams Ensemble e2e coverage#256265
Conversation
|
🤖 Jobs for this PR can be triggered through checkboxes. 🚧
ℹ️ To trigger the CI, please tick the checkbox below 👇
|
|
Pinging @elastic/obs-onboarding-team (Team:obs-onboarding) |
…2e-tests-for-wired-streams-onboarding-flows
| const streamsValidation = new StreamsValidationPage(page); | ||
| await streamsValidation.waitForStreamsToLoad(); | ||
| await streamsValidation.assertStreamDocCountGreaterThanZero('logs.otel'); | ||
| } else if (!isLogsEssentialsMode) { |
There was a problem hiding this comment.
Huh, actually what about logs essentials + wired streams?
There was a problem hiding this comment.
Sorry, had to quadruple check that.
Actually it's correct(kind of 😄 ). In Logs Essentials we don't rely on dashboards, we just rely on logs in the discover, same as in wired streams mode ( plus we rely on streams page). So when it falls under Wired streams category it skips the dashboard checks we do in stateful/serverless.
That also means we don't check any dashboards in wired streams flows in those tests. Initially I had a thought that we don't need to do this cause we don't touch metrics when enabling wired streams but now I am thinking that maybe it's also worth checking just to be safe.
WDYT?
There was a problem hiding this comment.
Sounds good, let's leave a comment though since these relationships are hard to figure out later on.
| await hostsOverviewPage.assertHostCpuNotEmpty(hostname); | ||
| } else { | ||
| await otelHostFlowPage.clickLogsExplorationCTA(); | ||
|
|
There was a problem hiding this comment.
You did this change in a bunch of places, it looks really good to me but I wonder why it was done with a dynamic import before - is there some race condition / circiular import thingy happening? Or was it just laziness and you are cleaning up here?
There was a problem hiding this comment.
I was basically cleaning up after myself. I don't remember why I did it that way. When I saw it I tested the static import and it worked just fine so I cleaned up the rest.
|
|
||
| if (!isLogsEssentialsMode) { | ||
| if (useWiredStreams && !isLogsEssentialsMode) { | ||
| await kubernetesEAFlowPage.assertReceivedDataIndicatorKubernetes(); |
There was a problem hiding this comment.
I'm a bit worried about how big these branches are getting - can we somehow deduplicate? It's fine if not, but it seems like it's easy to forget about stuff this way
There was a problem hiding this comment.
I'll try to extract some code into helpers
| @@ -11,9 +11,6 @@ export class DiscoverValidationPage { | |||
| constructor(private page: Page) {} | |||
|
|
|||
| async waitForDiscoverToLoad() { | |||
There was a problem hiding this comment.
Are you sure this is safe?
There was a problem hiding this comment.
I removed it cause it was messing up the test from time to time.
Then I found that they discouraged this approach. Playwright now recommends web assertions, we do something similar there so I left it as-is.
|
Looks mostly good, left some questions |
…2e-tests-for-wired-streams-onboarding-flows
📝 WalkthroughWalkthroughAdds wired streams ingestion-mode support to onboarding E2E tests: new validation helpers and page objects for Discover and Streams, a WiredStreamsSelector component and fixture, plus test-spec branching to validate either wired-streams flows (Discover + Streams) or legacy flows. Changes
Sequence DiagramsequenceDiagram
participant Test as Test Spec
participant WiredSel as WiredStreamsSelector
participant Flow as Flow Page (AutoDetect/Otel/K8s)
participant DiscoverVal as DiscoverValidationPage
participant StreamsVal as StreamsValidationPage
Test->>Test: Read USE_WIRED_STREAMS env flag
alt wired streams enabled
Test->>WiredSel: selectWiredStreamsMode()
WiredSel->>Flow: toggle wired option / confirm modal
WiredSel-->>Test: wired mode active
Test->>Flow: trigger Explore Logs / open custom explore popup?
Flow-->>Test: popup page (optional)
Test->>DiscoverVal: assertDiscoverHasData(assertHitCount?)
DiscoverVal->>DiscoverVal: wait for unified histogram / assert hits
DiscoverVal-->>Test: discover validated
Test->>StreamsVal: assertStreamHasData(streamName)
StreamsVal->>StreamsVal: wait for streams table / assert doc count
StreamsVal-->>Test: stream validated
else wired streams disabled
Test->>Flow: proceed with existing flow
alt logs essentials
Test->>DiscoverVal: assertDiscoverHasData()
DiscoverVal-->>Test: discover validated
else non-essentials
Test->>Flow: navigate to Host/Cluster overview
Flow-->>Test: dashboard validations
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/lib/validation_helpers.ts (1)
12-26: Consider adding explicit return types for exported async functions.Per coding guidelines, exported functions should have explicit return types. While TypeScript can infer
Promise<void>, adding it explicitly improves API clarity.♻️ Suggested improvement
-export async function assertDiscoverHasData(page: Page, { assertHitCount = false } = {}) { +export async function assertDiscoverHasData(page: Page, { assertHitCount = false } = {}): Promise<void> { const discoverValidation = new DiscoverValidationPage(page); await discoverValidation.waitForDiscoverToLoad(); await discoverValidation.assertHasAnyLogData(); if (assertHitCount) { await discoverValidation.assertHitCountGreaterThanZero(); } } -export async function assertStreamHasData(page: Page, streamName: string) { +export async function assertStreamHasData(page: Page, streamName: string): Promise<void> { await page.goto(`${process.env.KIBANA_BASE_URL}/app/streams`); const streamsValidation = new StreamsValidationPage(page); await streamsValidation.waitForStreamsToLoad(); await streamsValidation.assertStreamDocCountGreaterThanZero(streamName); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/lib/validation_helpers.ts` around lines 12 - 26, Add explicit return types to the two exported async functions in validation_helpers.ts: change the signatures of assertDiscoverHasData and assertStreamHasData to declare they return Promise<void>. Update both function declarations so the exported API clearly states Promise<void> as the return type (e.g., async function assertDiscoverHasData(...): Promise<void> and async function assertStreamHasData(...): Promise<void>).x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/pages/streams_validation.page.ts (1)
25-27: Optional: Extract shared numeric parsing helper.The hit/doc count parsing logic (regex match for digits with commas, replace commas, parse to int) is duplicated between
DiscoverValidationPage.assertHitCountGreaterThanZeroandStreamsValidationPage.assertStreamDocCountGreaterThanZero. Consider extracting a small shared utility.♻️ Example extraction
// In a shared utility file or at the top of one of these files: const parseFormattedNumber = (text: string): number => { const match = text.match(/[\d,]+/); return parseInt((match?.[0] ?? '0').replace(/,/g, ''), 10); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/pages/streams_validation.page.ts` around lines 25 - 27, Extract the duplicated numeric parsing logic into a small shared helper (e.g., parseFormattedNumber) and replace the inline regex+replace+parseInt in both DiscoverValidationPage.assertHitCountGreaterThanZero and StreamsValidationPage.assertStreamDocCountGreaterThanZero with calls to that helper; implement parseFormattedNumber to accept the raw text, match /[\d,]+/, strip commas and return parseInt(..., 10), and place the helper in a shared utility module (or at the top of one of the files) so both pages import and use it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/auto_detect.spec.ts`:
- Around line 84-90: Move the popup waiter so it is registered before the action
that opens the new tab: in the non-logs-essentials branch, call
page.waitForEvent('popup') and store the resulting promise, then call
autoDetectFlowPage.clickAutoDetectSystemIntegrationCTA(), and finally construct
the HostDetailsPage from the awaited popup promise (HostDetailsPage(await
popupPromise)). This ensures the event listener for the popup is attached before
invoking clickAutoDetectSystemIntegrationCTA() and only exists in the branch
that opens HostDetailsPage.
---
Nitpick comments:
In
`@x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/lib/validation_helpers.ts`:
- Around line 12-26: Add explicit return types to the two exported async
functions in validation_helpers.ts: change the signatures of
assertDiscoverHasData and assertStreamHasData to declare they return
Promise<void>. Update both function declarations so the exported API clearly
states Promise<void> as the return type (e.g., async function
assertDiscoverHasData(...): Promise<void> and async function
assertStreamHasData(...): Promise<void>).
In
`@x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/pages/streams_validation.page.ts`:
- Around line 25-27: Extract the duplicated numeric parsing logic into a small
shared helper (e.g., parseFormattedNumber) and replace the inline
regex+replace+parseInt in both
DiscoverValidationPage.assertHitCountGreaterThanZero and
StreamsValidationPage.assertStreamDocCountGreaterThanZero with calls to that
helper; implement parseFormattedNumber to accept the raw text, match /[\d,]+/,
strip commas and return parseInt(..., 10), and place the helper in a shared
utility module (or at the top of one of the files) so both pages import and use
it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 9f9b9fe7-cf30-496c-8033-6de37a8fa4ec
📒 Files selected for processing (12)
x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/lib/validation_helpers.tsx-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/auto_detect.spec.tsx-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/fixtures/base_page.tsx-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/host_otel.spec.tsx-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/kubernetes_ea.spec.tsx-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/kubernetes_otel.spec.tsx-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/components/wired_streams_selector.component.tsx-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/pages/auto_detect_flow.page.tsx-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/pages/discover_validation.page.tsx-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/pages/kubernetes_ea_flow.page.tsx-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/pages/otel_kubernetes_flow.page.tsx-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/pages/streams_validation.page.ts
💤 Files with no reviewable changes (2)
- x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/pages/otel_kubernetes_flow.page.ts
- x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/pages/kubernetes_ea_flow.page.ts
| await autoDetectFlowPage.clickAutoDetectSystemIntegrationCTA(); | ||
|
|
||
| await page.goto(`${process.env.KIBANA_BASE_URL}/app/discover`); | ||
| /** | ||
| * Host Details pages open in a new tab, so it | ||
| * needs to be captured using the `popup` event. | ||
| */ | ||
| const hostDetailsPage = new HostDetailsPage(await page.waitForEvent('popup')); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/auto_detect.spec.ts | sed -n '75,110p'Repository: elastic/kibana
Length of output: 1867
🌐 Web query:
Playwright waitForEvent popup race condition timing before click
💡 Result:
In Playwright, waitForEvent('popup') can “miss” the popup if you start waiting after the user action fires window.open() (classic timing/race issue). The fix is to arm the event listener first, then trigger the click/keypress.
Recommended (Playwright “event trap” pattern)
const popupPromise = page.waitForEvent('popup'); // no await here
await page.getByText('open the popup').click();
const popup = await popupPromise;This is the pattern shown in the official docs. [1]
Equivalent pattern with Promise.all
const [popup] = await Promise.all([
page.waitForEvent('popup'),
page.getByText('open the popup').click(),
]);This prevents the popup from opening before the wait is registered. [2]
If it’s a new tab/page (not necessarily a “popup”)
Sometimes the more reliable event is on the context:
const pagePromise = context.waitForEvent('page'); // arm first
await page.getByRole('link', { name: 'Open' }).click();
const newPage = await pagePromise;(Discussed in common Playwright guidance around “handling new pages”.) [2]
Rule of thumb: create the waitForEvent promise before the click/keypress that opens the window, and only await it afterwards. [1][2]
Sources: [1] Playwright Events docs, [2] Stack Overflow discussion of waitForEvent('popup') timing and patterns.
Register the popup waiter before clicking the CTA.
page.waitForEvent('popup') is attached after clickAutoDetectSystemIntegrationCTA(), so a fast new tab can be missed and this branch will hang intermittently. Playwright requires the event listener to be subscribed before the action that opens the popup. Move the popup waiter into the non-logs-essentials branch only, since that's where the Host Details page opens.
🛠️ Suggested fix
- await autoDetectFlowPage.clickAutoDetectSystemIntegrationCTA();
-
- /**
- * Host Details pages open in a new tab, so it
- * needs to be captured using the `popup` event.
- */
- const hostDetailsPage = new HostDetailsPage(await page.waitForEvent('popup'));
-
if (!isLogsEssentialsMode) {
+ /**
+ * Host Details opens in a new tab, so subscribe before clicking.
+ */
+ const popupPromise = page.waitForEvent('popup');
+ await autoDetectFlowPage.clickAutoDetectSystemIntegrationCTA();
+ const hostDetailsPage = new HostDetailsPage(await popupPromise);
await hostDetailsPage.assertCpuPercentageNotEmpty();
} else {
+ await autoDetectFlowPage.clickAutoDetectSystemIntegrationCTA();
await autoDetectFlowPage.assertReceivedDataIndicator();
await page.goto(`${process.env.KIBANA_BASE_URL}/app/discover`);
await assertDiscoverHasData(page);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/auto_detect.spec.ts`
around lines 84 - 90, Move the popup waiter so it is registered before the
action that opens the new tab: in the non-logs-essentials branch, call
page.waitForEvent('popup') and store the resulting promise, then call
autoDetectFlowPage.clickAutoDetectSystemIntegrationCTA(), and finally construct
the HostDetailsPage from the awaited popup promise (HostDetailsPage(await
popupPromise)). This ensures the event listener for the popup is attached before
invoking clickAutoDetectSystemIntegrationCTA() and only exists in the branch
that opens HostDetailsPage.
📝 WalkthroughWalkthroughThis PR introduces wired streams support to observability onboarding E2E tests by adding validation helpers, a wired streams selector component, and conditional test flows. Multiple test files are updated to check a USE_WIRED_STREAMS environment variable and execute alternate validation paths using new assertion helpers and page objects. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as E2E Test
participant WS as WiredStreamsSelector
participant UI as Onboarding UI
participant Discover as Discover Page
participant Streams as Streams Page
participant Helper as Validation Helpers
Test->>WS: selectWiredStreamsMode()
WS->>UI: Wait for ingestion mode selector
WS->>UI: Click wired streams option
WS->>UI: Check for enable modal (5s timeout)
alt Modal Present
WS->>UI: Click confirm button
WS->>UI: Wait for enabling spinner
WS->>UI: Wait for spinner to disappear (60s)
else Modal Not Present
Note over WS: Wired streams already enabled
end
WS->>UI: Wait for wired streams description (60s)
Test->>Test: Navigate through onboarding
Test->>Discover: Open Discover page
Test->>Helper: assertDiscoverHasData(page, options)
Helper->>Discover: Wait for page load
Helper->>Discover: Assert log data present
Helper-->>Test: Validation complete
Test->>Streams: Navigate to Streams page
Test->>Helper: assertStreamHasData(page, streamName)
Helper->>Streams: Wait for streams to load
Helper->>Streams: Assert stream document count > 0
Helper-->>Test: Validation complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/pages/discover_validation.page.ts (1)
35-44: Declare the new page-object method's return type.Please make this
: Promise<void>so the exportedDiscoverValidationPageAPI does not rely on inference.♻️ Suggested change
- async assertHitCountGreaterThanZero() { + async assertHitCountGreaterThanZero(): Promise<void> {As per coding guidelines, "Prefer explicit return types for public APIs and exported functions in TypeScript."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/pages/discover_validation.page.ts` around lines 35 - 44, The method assertHitCountGreaterThanZero currently relies on TypeScript inference for its return type; update its signature to explicitly declare the return type as Promise<void> (i.e., change async assertHitCountGreaterThanZero() to async assertHitCountGreaterThanZero(): Promise<void>) so the exported DiscoverValidationPage API has an explicit public return type.x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/kubernetes_ea.spec.ts (1)
66-76: Prefer condition-based polling over these new fixed sleeps.The added 2m/5m waits make the wired path slow on every pass and still guess at ingestion timing. If possible, drive this off a retryable UI/API condition and let the Discover/Streams assertions do the waiting instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/kubernetes_ea.spec.ts` around lines 66 - 76, Remove the fixed page.waitForTimeout(2 * 60000) and page.waitForTimeout(5 * 60000) sleeps and replace them with condition-based polling: repeatedly check a deterministic UI/API condition (e.g., call kubernetesEAFlowPage.assertReceivedDataIndicatorKubernetes() or try navigating/calling assertDiscoverHasData and assertStreamHasData in a retry loop) until success or timeout, then proceed to clickExploreLogsCTA() or navigate to Discover; ensure the retry uses a short interval and a max timeout so the wired path finishes as soon as data is available and the existing assertDiscoverHasData/assertStreamHasData methods drive the waiting rather than fixed sleeps.x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/pages/streams_validation.page.ts (1)
13-29: Type the exported streams page object explicitly.This new shared POM should make the injected
pagereadonly and declarePromise<void>on its public methods.♻️ Suggested change
- constructor(private page: Page) { + constructor(private readonly page: Page) { this.streamsTable = this.page.getByTestId('streamsTable'); } - async waitForStreamsToLoad() { + async waitForStreamsToLoad(): Promise<void> { await this.streamsTable.waitFor({ state: 'visible', timeout: 60000 }); } - async assertStreamDocCountGreaterThanZero(streamName: string) { + async assertStreamDocCountGreaterThanZero(streamName: string): Promise<void> { const docCount = this.page.locator(`[data-test-subj="streamsDocCount-${streamName}"]`); await docCount.waitFor({ state: 'visible', timeout: 60000 });As per coding guidelines, "Prefer explicit return types for public APIs and exported functions in TypeScript." and "Prefer readonly and as const for immutable structures in TypeScript."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/pages/streams_validation.page.ts` around lines 13 - 29, The exported streams page object should declare explicit types and readonly immutability: mark the injected page as a readonly Page (change the constructor parameter to readonly page: Page and/or declare a readonly private field), add explicit types for class properties like streamsTable (e.g., readonly streamsTable: Locator), and give public methods waitForStreamsToLoad and assertStreamDocCountGreaterThanZero explicit return types of Promise<void>; update the constructor, property declarations, and the signatures of waitForStreamsToLoad and assertStreamDocCountGreaterThanZero to reflect these types.x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/lib/validation_helpers.ts (1)
12-25: Make these shared helpers explicit typed exports.These are now part of the common test surface. Please type the options/return values explicitly and export them as
constarrows instead of relying on inferred function declarations.♻️ Suggested change
+type AssertDiscoverHasDataOptions = Readonly<{ + assertHitCount?: boolean +}> + -export async function assertDiscoverHasData(page: Page, { assertHitCount = false } = {}) { +export const assertDiscoverHasData = async ( + page: Page, + { assertHitCount = false }: AssertDiscoverHasDataOptions = {} +): Promise<void> => { const discoverValidation = new DiscoverValidationPage(page); await discoverValidation.waitForDiscoverToLoad(); await discoverValidation.assertHasAnyLogData(); if (assertHitCount) { await discoverValidation.assertHitCountGreaterThanZero(); } -} +} -export async function assertStreamHasData(page: Page, streamName: string) { +export const assertStreamHasData = async ( + page: Page, + streamName: string +): Promise<void> => { await page.goto(`${process.env.KIBANA_BASE_URL}/app/streams`); const streamsValidation = new StreamsValidationPage(page); await streamsValidation.waitForStreamsToLoad(); await streamsValidation.assertStreamDocCountGreaterThanZero(streamName); -} +}As per coding guidelines, "Prefer explicit return types for public APIs and exported functions in TypeScript." and "Prefer const arrow functions in TypeScript."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/lib/validation_helpers.ts` around lines 12 - 25, The exported helpers assertDiscoverHasData and assertStreamHasData should be converted to explicitly typed const arrow exports and given explicit parameter and return types: change to export const assertDiscoverHasData: (page: Page, opts?: { assertHitCount?: boolean }) => Promise<void> = async (page, { assertHitCount = false } = {}) => { ... } and export const assertStreamHasData: (page: Page, streamName: string) => Promise<void> = async (page, streamName) => { ... }; ensure any option object type is declared inline or as a small interface/type alias and that the functions still call DiscoverValidationPage/StreamsValidationPage methods as before.x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/components/wired_streams_selector.component.ts (1)
39-39: Add the explicit return type on this public helper.
selectWiredStreamsModeis part of the exported page-object API, so it should declarePromise<void>explicitly.Suggested fix
- public async selectWiredStreamsMode() { + public async selectWiredStreamsMode(): Promise<void> {As per coding guidelines, "Prefer explicit return types for public APIs and exported functions in TypeScript."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/components/wired_streams_selector.component.ts` at line 39, The public helper method selectWiredStreamsMode lacks an explicit return type; update its signature to declare Promise<void> (e.g., change "public async selectWiredStreamsMode()" to explicitly return Promise<void>) so the exported page-object API has a clear TypeScript return type; ensure the change is made on the selectWiredStreamsMode method in wired_streams_selector.component.ts and that no other behavior is altered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/auto_detect.spec.ts`:
- Around line 77-82: When useWiredStreams is true we currently only handle the
popup path and then silently continue when hasCustomLogsExploreButtons() is
false; update the branch so you explicitly assert or exercise the non-popup
Explore Logs path: call autoDetectFlowPage.hasCustomLogsExploreButtons(), and if
true keep the current flow (clickCustomLogsExploreInPopup() and
assertDiscoverHasData(popupPage)), otherwise call the non-popup action (e.g.
autoDetectFlowPage.clickExploreLogs() or the equivalent method on
autoDetectFlowPage) and then call assertDiscoverHasData(...) on the returned or
current page; this ensures either an explicit assertion that the custom buttons
exist or a fallback to the normal Explore Logs action instead of skipping
Discover coverage.
In
`@x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/kubernetes_otel.spec.ts`:
- Around line 49-54: The helm repository snippet is being read before switching
to wired mode, producing a mixed-script; move the wired-mode toggle so
wiredStreamsSelector.selectWiredStreamsMode() runs before calling
otelKubernetesFlowPage.copyHelmRepositorySnippetToClipboard() and reading
helmRepoSnippet (the variable), ensuring both helmRepoSnippet and
installStackSnippet are captured after useWiredStreams is applied.
In
`@x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/components/wired_streams_selector.component.ts`:
- Around line 48-65: The current try/catch around
enableWiredStreamsModal.waitFor also swallows errors from
enableWiredStreamsConfirmButton.click() and enablingSpinner waits; change the
logic so only the modal visibility wait is optional: call await
this.enableWiredStreamsModal.waitFor(...) inside a try/catch that treats failure
as "modal did not appear" and returns/continues, but perform
this.enableWiredStreamsConfirmButton.click() and the
enablingSpinner.waitFor(...) calls outside that catch so any click/spinner
failures propagate; reference enableWiredStreamsModal,
enableWiredStreamsConfirmButton, and enablingSpinner to locate and update the
code accordingly.
---
Nitpick comments:
In
`@x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/lib/validation_helpers.ts`:
- Around line 12-25: The exported helpers assertDiscoverHasData and
assertStreamHasData should be converted to explicitly typed const arrow exports
and given explicit parameter and return types: change to export const
assertDiscoverHasData: (page: Page, opts?: { assertHitCount?: boolean }) =>
Promise<void> = async (page, { assertHitCount = false } = {}) => { ... } and
export const assertStreamHasData: (page: Page, streamName: string) =>
Promise<void> = async (page, streamName) => { ... }; ensure any option object
type is declared inline or as a small interface/type alias and that the
functions still call DiscoverValidationPage/StreamsValidationPage methods as
before.
In
`@x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/kubernetes_ea.spec.ts`:
- Around line 66-76: Remove the fixed page.waitForTimeout(2 * 60000) and
page.waitForTimeout(5 * 60000) sleeps and replace them with condition-based
polling: repeatedly check a deterministic UI/API condition (e.g., call
kubernetesEAFlowPage.assertReceivedDataIndicatorKubernetes() or try
navigating/calling assertDiscoverHasData and assertStreamHasData in a retry
loop) until success or timeout, then proceed to clickExploreLogsCTA() or
navigate to Discover; ensure the retry uses a short interval and a max timeout
so the wired path finishes as soon as data is available and the existing
assertDiscoverHasData/assertStreamHasData methods drive the waiting rather than
fixed sleeps.
In
`@x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/components/wired_streams_selector.component.ts`:
- Line 39: The public helper method selectWiredStreamsMode lacks an explicit
return type; update its signature to declare Promise<void> (e.g., change "public
async selectWiredStreamsMode()" to explicitly return Promise<void>) so the
exported page-object API has a clear TypeScript return type; ensure the change
is made on the selectWiredStreamsMode method in
wired_streams_selector.component.ts and that no other behavior is altered.
In
`@x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/pages/discover_validation.page.ts`:
- Around line 35-44: The method assertHitCountGreaterThanZero currently relies
on TypeScript inference for its return type; update its signature to explicitly
declare the return type as Promise<void> (i.e., change async
assertHitCountGreaterThanZero() to async assertHitCountGreaterThanZero():
Promise<void>) so the exported DiscoverValidationPage API has an explicit public
return type.
In
`@x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/pages/streams_validation.page.ts`:
- Around line 13-29: The exported streams page object should declare explicit
types and readonly immutability: mark the injected page as a readonly Page
(change the constructor parameter to readonly page: Page and/or declare a
readonly private field), add explicit types for class properties like
streamsTable (e.g., readonly streamsTable: Locator), and give public methods
waitForStreamsToLoad and assertStreamDocCountGreaterThanZero explicit return
types of Promise<void>; update the constructor, property declarations, and the
signatures of waitForStreamsToLoad and assertStreamDocCountGreaterThanZero to
reflect these types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 5d8aefdd-ad67-4a49-8ac4-39bbc15d0b66
📒 Files selected for processing (12)
x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/lib/validation_helpers.tsx-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/auto_detect.spec.tsx-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/fixtures/base_page.tsx-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/host_otel.spec.tsx-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/kubernetes_ea.spec.tsx-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/kubernetes_otel.spec.tsx-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/components/wired_streams_selector.component.tsx-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/pages/auto_detect_flow.page.tsx-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/pages/discover_validation.page.tsx-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/pages/kubernetes_ea_flow.page.tsx-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/pages/otel_kubernetes_flow.page.tsx-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/pages/streams_validation.page.ts
💤 Files with no reviewable changes (2)
- x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/pages/kubernetes_ea_flow.page.ts
- x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/pages/otel_kubernetes_flow.page.ts
| if (useWiredStreams) { | ||
| if (await autoDetectFlowPage.hasCustomLogsExploreButtons()) { | ||
| const popupPage = await autoDetectFlowPage.clickCustomLogsExploreInPopup(); | ||
| await assertDiscoverHasData(popupPage, { assertHitCount: true }); | ||
| } | ||
| await assertStreamHasData(page, 'logs.ecs'); |
There was a problem hiding this comment.
Don't let wired-mode Discover coverage be skipped silently.
If hasCustomLogsExploreButtons() flips to false, this branch still passes after only checking the stream page. Please assert those buttons are present or fall back to the non-popup Explore Logs action so regressions in the user-facing Discover path are caught.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/auto_detect.spec.ts`
around lines 77 - 82, When useWiredStreams is true we currently only handle the
popup path and then silently continue when hasCustomLogsExploreButtons() is
false; update the branch so you explicitly assert or exercise the non-popup
Explore Logs path: call autoDetectFlowPage.hasCustomLogsExploreButtons(), and if
true keep the current flow (clickCustomLogsExploreInPopup() and
assertDiscoverHasData(popupPage)), otherwise call the non-popup action (e.g.
autoDetectFlowPage.clickExploreLogs() or the equivalent method on
autoDetectFlowPage) and then call assertDiscoverHasData(...) on the returned or
current page; this ensures either an explicit assertion that the custom buttons
exist or a fallback to the normal Explore Logs action instead of skipping
Discover coverage.
| await otelKubernetesFlowPage.copyHelmRepositorySnippetToClipboard(); | ||
| const helmRepoSnippet = (await page.evaluate('navigator.clipboard.readText()')) as string; | ||
|
|
||
| if (useWiredStreams) { | ||
| await wiredStreamsSelector.selectWiredStreamsMode(); | ||
| } |
There was a problem hiding this comment.
Switch to wired mode before reading the first snippet.
In wired runs, helmRepoSnippet still comes from the default path while installStackSnippet comes from wired mode. That builds a mixed script instead of the actual wired instructions.
💡 Suggested change
- await otelKubernetesFlowPage.copyHelmRepositorySnippetToClipboard();
- const helmRepoSnippet = (await page.evaluate('navigator.clipboard.readText()')) as string;
-
if (useWiredStreams) {
await wiredStreamsSelector.selectWiredStreamsMode();
}
+
+ await otelKubernetesFlowPage.copyHelmRepositorySnippetToClipboard();
+ const helmRepoSnippet = (await page.evaluate('navigator.clipboard.readText()')) as string;
await otelKubernetesFlowPage.copyInstallStackSnippetToClipboard();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/kubernetes_otel.spec.ts`
around lines 49 - 54, The helm repository snippet is being read before switching
to wired mode, producing a mixed-script; move the wired-mode toggle so
wiredStreamsSelector.selectWiredStreamsMode() runs before calling
otelKubernetesFlowPage.copyHelmRepositorySnippetToClipboard() and reading
helmRepoSnippet (the variable), ensuring both helmRepoSnippet and
installStackSnippet are captured after useWiredStreams is applied.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/components/wired_streams_selector.component.ts`:
- Around line 48-51: The current waitFor calls on this.enableWiredStreamsModal
swallow all errors via catch(() => false), which masks real Playwright failures;
update the code so each waitFor call (the one assigned to modalVisible and the
second waitFor at lines 59–62) catches errors and returns false only when
error.name === 'TimeoutError' (or equivalent TimeoutError check) and otherwise
rethrows the error so non-timeout failures (e.g., TargetClosedError) are not
suppressed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 5243771d-606e-4acb-b155-b3a8696784d6
📒 Files selected for processing (4)
x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/lib/validation_helpers.tsx-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/components/wired_streams_selector.component.tsx-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/pages/discover_validation.page.tsx-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/pages/streams_validation.page.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/pages/discover_validation.page.ts
- x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/lib/validation_helpers.ts
- x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/pages/streams_validation.page.ts
| const modalVisible = await this.enableWiredStreamsModal | ||
| .waitFor({ state: 'visible', timeout: 5000 }) | ||
| .then(() => true) | ||
| .catch(() => false); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/components/wired_streams_selector.component.tsRepository: elastic/kibana
Length of output: 3196
🏁 Script executed:
# Search for similar waitFor().then().catch() patterns in the observability onboarding e2e tests
rg -A 2 "\.waitFor\(" x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright --type ts -A 2 | head -50Repository: elastic/kibana
Length of output: 4559
🏁 Script executed:
# Check Playwright documentation pattern for optional element detection
# Also search for similar patterns in other test files
fd "\.component\.ts" x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright --exec rg -l "waitFor" {}Repository: elastic/kibana
Length of output: 196
🏁 Script executed:
cat -n x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/lib/helpers.ts | head -60Repository: elastic/kibana
Length of output: 1637
🏁 Script executed:
# Check Playwright's TimeoutError and other error types that can be thrown
web_search "Playwright waitFor timeout error types"Repository: elastic/kibana
Length of output: 108
🏁 Script executed:
rg "waitFor.*catch" x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright --type ts -B 2 -A 2Repository: elastic/kibana
Length of output: 761
🏁 Script executed:
# Check what Playwright error types are used in the codebase
rg "TimeoutError|Error.*Playwright" x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright --type tsRepository: elastic/kibana
Length of output: 40
🏁 Script executed:
# Look for better error handling patterns with waitFor in observability tests
rg "waitFor.*\{" x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright --type ts -B 1 -A 3 | grep -A 5 "catch"Repository: elastic/kibana
Length of output: 2430
🏁 Script executed:
# Check if there's any Playwright error type checking in the codebase
rg "catch.*error|catch.*err" x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright --type ts -B 2 -A 2Repository: elastic/kibana
Length of output: 40
🏁 Script executed:
# Look at broader Playwright patterns in Security solution tests (mentioned in learnings)
fd "\.cy\.ts$" x-pack/solutions/security/test/security_solution_cypress -type f | head -3 | xargs -I {} grep -l "waitFor\|cy.wait" {}Repository: elastic/kibana
Length of output: 228
🏁 Script executed:
# Search for explicit TimeoutError handling in Playwright tests across kibana
rg "TimeoutError" --type ts x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright 2>/dev/null || echo "No TimeoutError found"Repository: elastic/kibana
Length of output: 79
🏁 Script executed:
# Check how Playwright is used in other parts of observability to understand established patterns
fd "component.ts" x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/components/ --type f | xargs wc -l | tail -1Repository: elastic/kibana
Length of output: 68
🏁 Script executed:
# Look at a few other component files to see if broad catch() is a widespread pattern
ls -la x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/components/Repository: elastic/kibana
Length of output: 404
🏁 Script executed:
cat -n x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/components/header_bar.component.tsRepository: elastic/kibana
Length of output: 861
🏁 Script executed:
cat -n x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/components/space_selector.component.tsRepository: elastic/kibana
Length of output: 885
Restrict catch() to TimeoutError only; avoid masking real Playwright failures.
Both catch(() => false) and catch(() => undefined) blocks suppress all errors thrown by waitFor(), not just the expected timeout when optional UI doesn't appear. Playwright can also throw TargetClosedError (page closed), navigation errors, or other protocol failures—which these blocks silently treat as "UI missing". This masks the real cause and can turn it into a slower, less actionable timeout later at line 69.
For detecting optional UI, explicitly check for TimeoutError only:
const modalVisible = await this.enableWiredStreamsModal
.waitFor({ state: 'visible', timeout: 5000 })
.then(() => true)
.catch((error) => {
if (error.name === 'TimeoutError') return false;
throw error;
});
Apply the same fix to lines 59–62.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@x-pack/solutions/observability/plugins/observability_onboarding/e2e/playwright/stateful/pom/components/wired_streams_selector.component.ts`
around lines 48 - 51, The current waitFor calls on this.enableWiredStreamsModal
swallow all errors via catch(() => false), which masks real Playwright failures;
update the code so each waitFor call (the one assigned to modalVisible and the
second waitFor at lines 59–62) catches errors and returns false only when
error.name === 'TimeoutError' (or equivalent TimeoutError check) and otherwise
rethrows the error so non-timeout failures (e.g., TargetClosedError) are not
suppressed.
|
@rStelmach Thanks for the cleanup! This looks mostly good to me, I didn't run locally, I tried to check the ensemble output but I couldn't find any actual output in the stored artifacts, is that normal? Can I inspect the logs/screenshots from the run somewhere? Also seems like the fleet suites failed, is that expected? Doesn't look related to your changes at least |
|
@flash1293 Those fleet tests have been failing for couple weeks now. They run in the same nightly workflow but we don't own them, it's unrelated to any of our recent changes. |
|
Ah, perfect, thanks! |
…2e-tests-for-wired-streams-onboarding-flows
…2e-tests-for-wired-streams-onboarding-flows
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]
History
|
…lastic#256265) ## Summary Add wired streams Ensemble e2e coverage for Observability onboarding flows. The Ensemble stories enable wired streams by setting `USE_WIRED_STREAMS=true` for the existing Playwright specs. On the Kibana side, the specs select the Wired Streams mode in the onboarding UI, generate wired snippets, then validate that data is visible. ## Context Wired streams changes the destination for log data: - OTel flows route logs to `logs.otel` - Elastic Agent flows route logs to `logs.ecs` The UI provides an "Explore logs" path that navigates to Discover with a wired data view (for example `logs.ecs,logs.ecs.*`). We also validate on `/app/streams` to prove documents landed in the expected wired stream. ## Ensemble run https://github.com/elastic/ensemble/actions/runs/22683342478 https://github.com/elastic/ensemble/actions/runs/22724085715 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added end-to-end validation for wired streams mode in onboarding flows. * New helpers to verify data presence in Discover and Streams views. * Extended test fixtures and page objects to support wired-streams and additional validation paths across onboarding scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
…256265) ## Summary Add wired streams Ensemble e2e coverage for Observability onboarding flows. The Ensemble stories enable wired streams by setting `USE_WIRED_STREAMS=true` for the existing Playwright specs. On the Kibana side, the specs select the Wired Streams mode in the onboarding UI, generate wired snippets, then validate that data is visible. ## Context Wired streams changes the destination for log data: - OTel flows route logs to `logs.otel` - Elastic Agent flows route logs to `logs.ecs` The UI provides an "Explore logs" path that navigates to Discover with a wired data view (for example `logs.ecs,logs.ecs.*`). We also validate on `/app/streams` to prove documents landed in the expected wired stream. ## Ensemble run https://github.com/elastic/ensemble/actions/runs/22683342478 https://github.com/elastic/ensemble/actions/runs/22724085715 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added end-to-end validation for wired streams mode in onboarding flows. * New helpers to verify data presence in Discover and Streams views. * Extended test fixtures and page objects to support wired-streams and additional validation paths across onboarding scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Summary
Add wired streams Ensemble e2e coverage for Observability onboarding flows.
The Ensemble stories enable wired streams by setting
USE_WIRED_STREAMS=truefor the existing Playwright specs.On the Kibana side, the specs select the Wired Streams mode in the onboarding UI, generate wired snippets, then validate that data is visible.
Context
Wired streams changes the destination for log data:
logs.otellogs.ecsThe UI provides an "Explore logs" path that navigates to Discover with a wired data view (for example
logs.ecs,logs.ecs.*).We also validate on
/app/streamsto prove documents landed in the expected wired stream.Ensemble run
https://github.com/elastic/ensemble/actions/runs/22683342478
https://github.com/elastic/ensemble/actions/runs/22724085715
Summary by CodeRabbit