-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Logs UI] Set streamLive false in URL state when arriving from link-to #56329
Conversation
Pinging @elastic/kibana-test-triage (failed-test) |
Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui) |
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking out loud.
I understand the reason why this test was flaky was because the streamLive
parameter in the URL sometimes was false
, and sometimes wasn't there, and the test was only covering the false
case.
streamLive
is a 3-state boolean (true
, false
, and not-there). In practice false
and not-there have the same effect so we can remove one of them.
This PR removes false
leaving only two states: true
and not-there. I think that makes it a bit harder to reason about it that if it had true
and false
as valid states.
I haven't dug into the code, but I think that could be made with this change.
-const mapToStreamLiveUrlState = (value: any) => (typeof value === 'boolean' ? value : undefined);
+const mapToStreamLiveUrlState = (value: any) => (typeof value === 'boolean' ? value : false);
Maybe you tried that already and it didn't work correctly because sometimes it wasn't being called in the right order during initialisation and the test was still flaky, but I think it might be worth a try. Otherwise I can go ahead and approve the PR
I would agree that we should prefer |
I think that happens on page load, when the URL has no values. |
Just pushed another approach, I think this should actually fix it for real. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Perfect!
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
elastic#56329) * [Logs UI] Remove streamLive false from URL state * Force false livestream boolean from link-to * Update type def * Update snapshots * Fix snapshot typo * Fix snapshot again * Fix node test snapshot Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
elastic#56329) * [Logs UI] Remove streamLive false from URL state * Force false livestream boolean from link-to * Update type def * Update snapshots * Fix snapshot typo * Fix snapshot again * Fix node test snapshot Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
#56329) (#57115) * [Logs UI] Remove streamLive false from URL state * Force false livestream boolean from link-to * Update type def * Update snapshots * Fix snapshot typo * Fix snapshot again * Fix node test snapshot Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
#56329) (#57114) * [Logs UI] Remove streamLive false from URL state * Force false livestream boolean from link-to * Update type def * Update snapshots * Fix snapshot typo * Fix snapshot again * Fix node test snapshot Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
…t-state * upstream/master: (96 commits) top nav ts arg support (elastic#56984) [SIEM][detection engine] Limit network rules to filebeat source semantics (elastic#57130) Add docs for alerting and action settings (elastic#57035) Add Test to Verify Endpoint App Landing Page (elastic#57129) Update `markdown-to-jsx` (`6.9.3` → `6.11.0`) and `url-parse` (`1.4.4` → `1.4.7`) dependencies. (elastic#57126) chore(NA): removes use of parallel option in the terser minimizer (elastic#57077) [ML] New Platform server shim: update file data visualizer routes to use new platform router (elastic#56972) Specifying valid licenses for the Graph feature (elastic#55911) [APM][docs] Add troubleshooting for non-indexed fields (elastic#54948) [ML] DF Analytics creation: update schema definition for create route (elastic#56979) Remove Kibana a11y guide in favor of EUI (elastic#57021) [Logs UI] Set streamLive false in URL state when arriving from link-to (elastic#56329) [docs] Fix spaces api example json (elastic#50411) Add new config for filebeat index name (elastic#56920) [Metrics-UI] Fix toolbar popover for metrics table row (elastic#56796) Saved Objects testing (elastic#56965) Disabled categorization stats validation (elastic#57087) [Rollups] Server NP migration (elastic#55606) [Metrics UI] Limit group by selector to only 2 fields (elastic#56800) fix auto closing new vis modal when navigating to lens or when navigating away with browser history (elastic#56998) ...
* master: (96 commits) top nav ts arg support (elastic#56984) [SIEM][detection engine] Limit network rules to filebeat source semantics (elastic#57130) Add docs for alerting and action settings (elastic#57035) Add Test to Verify Endpoint App Landing Page (elastic#57129) Update `markdown-to-jsx` (`6.9.3` → `6.11.0`) and `url-parse` (`1.4.4` → `1.4.7`) dependencies. (elastic#57126) chore(NA): removes use of parallel option in the terser minimizer (elastic#57077) [ML] New Platform server shim: update file data visualizer routes to use new platform router (elastic#56972) Specifying valid licenses for the Graph feature (elastic#55911) [APM][docs] Add troubleshooting for non-indexed fields (elastic#54948) [ML] DF Analytics creation: update schema definition for create route (elastic#56979) Remove Kibana a11y guide in favor of EUI (elastic#57021) [Logs UI] Set streamLive false in URL state when arriving from link-to (elastic#56329) [docs] Fix spaces api example json (elastic#50411) Add new config for filebeat index name (elastic#56920) [Metrics-UI] Fix toolbar popover for metrics table row (elastic#56796) Saved Objects testing (elastic#56965) Disabled categorization stats validation (elastic#57087) [Rollups] Server NP migration (elastic#55606) [Metrics UI] Limit group by selector to only 2 fields (elastic#56800) fix auto closing new vis modal when navigating to lens or when navigating away with browser history (elastic#56998) ...
Summary
Closes #55652
Removes thestreamLive
param from the log position URL state whenever it's false, and only adds it back in when it's true.Forces the
streamLive
param tofalse
when replacing alink-to
search string in the logs stream URL.This fixes a flaky test.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.[ ] This was checked for cross-browser compatibility, including a check against IE11[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support[ ] Documentation was added for features that require explanation or tutorials[ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers