Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Logs UI] Make version dependent test only run for intended version. #188901

Merged
merged 13 commits into from
Sep 2, 2024
Merged

[Logs UI] Make version dependent test only run for intended version. #188901

merged 13 commits into from
Sep 2, 2024

Conversation

awahab07
Copy link
Contributor

@awahab07 awahab07 commented Jul 23, 2024 •
edited by kibanamachine
Loading

Fixes #163845

Summary

A test in 7.17 is skipped because an ES API in the later versions has changed. The PR branches the test to handle both cases, before and after the API change. The PR should be backported to 7.17 once merged in main.

@awahab07 awahab07 added the release_note:skip Skip the PR/issue when compiling release notes label Jul 23, 2024
@obltmachine
Copy link

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

@awahab07 awahab07 added the Team:obs-ux-logs Observability Logs User Experience Team label Jul 23, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-logs-team (Team:obs-ux-logs)

@weltenwort
Copy link
Member

thanks for tackling this, @awahab07! I wonder if it would make sense to have both variants of the tests tagged this way so that one that only runs when ES <8.10 with the old assertion and the other with ES >= 8.10. That should be possible with these tags, right?

@awahab07
Copy link
Contributor Author

I wonder if it would make sense to have both variants of the tests tagged this way so that one that only runs when ES <8.10 with the old assertion and the other with ES >= 8.10. That should be possible with these tags, right?

Yes, this should be possible and would be a neat way. I'll be pushing more changes soon.

@awahab07 awahab07 requested a review from a team as a code owner July 29, 2024 21:01
dmlemeshko
dmlemeshko reviewed Jul 30, 2024
View reviewed changes
Copy link
Member

@dmlemeshko dmlemeshko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using this.onlyEsVersion(esVersion) in particular case is not following the original use case and leads to extra filtering on FTR side. Let me know your thoughts on alternative I posted

x-pack/test/api_integration/apis/metrics_ui/log_entry_highlights.ts Outdated
Comment on lines 167 to 173
// All entries contain the highlights
entries.forEach((entry) => {
entry.columns.forEach((column) => {
if ('field' in column && 'highlights' in column && column.highlights.length > 0) {
expect(column.highlights).to.eql(['generate_test_data/simple_logs']);
}
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the test will look "cleaner" if version is get directly in test:

import semver from 'semver';
...
const kibanaServer = getService('kibanaServer');
const currentVersion = kibanaServer.version.get();
const expectation = semver.gte(currentVersion, '8.10.0') ? [highlightTerms] : highlightTerms.split(' ');

wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmlemeshko thanks for the suggestion. The test is dependent on ES version as there's a change in ES API. Will kibanaServer.version.get() work in this case?

Copy link
Member

@weltenwort weltenwort Aug 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @dmlemeshko, that indeed looks less verbose. But your example binds it to the Kibana version and not the ES version. Since our goal here is to have different behavior on ES compatibility tests this difference is important. Do you know of a way to do what you show in your example but with respect to the ES version?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Then you can use es service:

const es = getService('es');
const response = await es.info();
const version = response.body.version.number;

@awahab07
Copy link
Contributor Author

awahab07 commented Aug 8, 2024

@elasticmachine merge upstream

@awahab07
Copy link
Contributor Author

@elasticmachine merge upstream

@awahab07
Copy link
Contributor Author

@elasticmachine merge upstream

@awahab07
Copy link
Contributor Author

@elasticmachine merge upstream

@awahab07
Copy link
Contributor Author

@elasticmachine merge upstream

@awahab07
Copy link
Contributor Author

awahab07 commented Sep 2, 2024

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

weltenwort
weltenwort approved these changes Sep 2, 2024
View reviewed changes
Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very elegant, nice job. It's hard to test this in a realistic forward-compat test within this PR, so I guess we'll have to keep an eye on it once backported.

@awahab07 awahab07 added the bug Fixes for quality problems that affect the customer experience label Sep 2, 2024
@awahab07 awahab07 added backport:version Backport to applied version labels v7.17.24 labels Sep 2, 2024
@awahab07 awahab07 merged commit ccbef9f into elastic:main Sep 2, 2024
25 checks passed
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
❌ 7.17 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 188901

Questions ?

Please refer to the Backport tool documentation

@awahab07 awahab07 mentioned this pull request Sep 2, 2024
Merged
@awahab07
Copy link
Contributor Author

awahab07 commented Sep 2, 2024

💚 All backports created successfully

Status Branch Result
✅ 7.17

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

awahab07 added a commit that referenced this pull request Sep 4, 2024
@awahab07
[7.17] [Logs UI] Make version dependent test only run for intended ve…
2a12f7d 
…rsion. (#188901) (#191925)

# Backport

This will backport the following commits from `main` to `7.17`:
- [[Logs UI] Make version dependent test only run for intended version.
(#188901)](#188901)

<!--- Backport version: 8.9.8 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Abdul Wahab
Zahid","email":"awahab07@yahoo.com"},"sourceCommit":{"committedDate":"2024-09-02T12:24:27Z","message":"[Logs
UI] Make version dependent test only run for intended version.
(#188901)\n\nFixes #163845\r\n\r\n## Summary\r\n\r\nA test in 7.17 is
skipped because an ES API in the later versions has\r\nchanged. The PR
branches the test to handle both cases, before and after\r\nthe API
change. The PR should be backported to 7.17 once merged in
main.\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine
<elasticmachine@users.noreply.github.com>","sha":"ccbef9f9a5aec5a3abb5c7d4a3574c896bc55f8b","branchLabelMapping":{"^v8.16.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:skip","Team:obs-ux-logs","v8.16.0","v7.17.24","backport:version"],"number":188901,"url":"https://github.com/elastic/kibana/pull/188901","mergeCommit":{"message":"[Logs
UI] Make version dependent test only run for intended version.
(#188901)\n\nFixes #163845\r\n\r\n## Summary\r\n\r\nA test in 7.17 is
skipped because an ES API in the later versions has\r\nchanged. The PR
branches the test to handle both cases, before and after\r\nthe API
change. The PR should be backported to 7.17 once merged in
main.\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine
<elasticmachine@users.noreply.github.com>","sha":"ccbef9f9a5aec5a3abb5c7d4a3574c896bc55f8b"}},"sourceBranch":"main","suggestedTargetBranches":["7.17"],"targetPullRequestStates":[{"branch":"main","label":"v8.16.0","labelRegex":"^v8.16.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/188901","number":188901,"mergeCommit":{"message":"[Logs
UI] Make version dependent test only run for intended version.
(#188901)\n\nFixes #163845\r\n\r\n## Summary\r\n\r\nA test in 7.17 is
skipped because an ES API in the later versions has\r\nchanged. The PR
branches the test to handle both cases, before and after\r\nthe API
change. The PR should be backported to 7.17 once merged in
main.\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine
<elasticmachine@users.noreply.github.com>","sha":"ccbef9f9a5aec5a3abb5c7d4a3574c896bc55f8b"}},{"branch":"7.17","label":"v7.17.24","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmlemeshko dmlemeshko dmlemeshko left review comments

@weltenwort weltenwort weltenwort approved these changes

Assignees
No one assigned
Labels
backport:version Backport to applied version labels bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-logs Observability Logs User Experience Team v7.17.24 v8.16.0
Projects
None yet
Milestone
No milestone
7 participants
@awahab07 @obltmachine @elasticmachine @weltenwort @kibana-ci @kibanamachine @dmlemeshko