-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Migrate es_archives/lens/basic/ to kbnArchiver #108120
Conversation
Pinging @elastic/kibana-qa (Team:QA) |
@elasticmachine merge upstream |
2014ef7
to
201a89a
Compare
@elasticmachine merge upstream |
Keeps passing in my local. |
@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.
Without fully investigating and debugging why the timepicker didn't get set correctly, the changes I've proposed here should resolve the problem by setting the default time to the same as the time we would set the timepicker to. It shouldn't hurt at all if the timepicker is still set, but you could potentially remove all those calls (at least in the ones the failed in Jenkins like kibana/x-pack/test/functional/apps/lens/smokescreen.ts
) and the test will be faster.
await esArchiver.load('x-pack/test/functional/es_archives/lens/basic'); | ||
await kibanaServer.importExport.load( | ||
'x-pack/test/functional/fixtures/kbn_archiver/lens/lens_basic.json' | ||
); |
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.
The test failure on the last run and it's screenshot, shows that for some reason the end time in the timepicker didn't get set to the required date/time (in this file kibana/x-pack/test/functional/apps/lens/smokescreen.ts
);
✖ fail: lens app lens smokescreen tests should create a pie chart and switch to datatable
│ Error: expected '@timestamp per 30 days' to sort of equal '@timestamp per 3 hours'
│ + expected - actual
│
│ -@timestamp per 30 days
│ +@timestamp per 3 hours
I thought about adding code in the time_picker page object method setAbsoluteTimeRange (sp?) to check that it actually did get the time set correctly. But decided instead to suggest this change to make the tests faster by setting the default time for the timepicker. This eliminates the need to call PageObjects.lens.goToTimeRange()
); | |
); | |
// changing the timepicker default here saves us from having to set it in Discover (~8s) | |
await PageObjects.timePicker.setDefaultAbsoluteRangeViaUiSettings(); | |
await kibanaServer.uiSettings.update({'dateFormat:tz': 'UTC', }); |
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.
Using the timepicker here requires a couple other changes;
diff --git a/x-pack/test/functional/apps/lens/index.ts b/x-pack/test/functional/apps/lens/index.ts
index 0c3cbce23bd..7b26640738b 100644
--- a/x-pack/test/functional/apps/lens/index.ts
+++ b/x-pack/test/functional/apps/lens/index.ts
@@ -7,20 +7,24 @@
import { FtrProviderContext } from '../../ftr_provider_context';
-export default function ({ getService, loadTestFile }: FtrProviderContext) {
+export default function ({ getService, loadTestFile, getPageObjects }: FtrProviderContext) {
const browser = getService('browser');
const log = getService('log');
const esArchiver = getService('esArchiver');
const kibanaServer = getService('kibanaServer');
+ const PageObjects = getPageObjects(['timePicker',]);
describe('lens app', () => {
before(async () => {
log.debug('Starting lens before method');
await browser.setWindowSize(1280, 800);
- await esArchiver.load('x-pack/test/functional/es_archives/logstash_functional');
+ await esArchiver.loadIfNeeded('x-pack/test/functional/es_archives/logstash_functional');
await kibanaServer.importExport.load(
'x-pack/test/functional/fixtures/kbn_archiver/lens/lens_basic.json'
);
+ // changing the timepicker default here saves us from having to set it in Discover (~8s)
+ await PageObjects.timePicker.setDefaultAbsoluteRangeViaUiSettings();
+ await kibanaServer.uiSettings.update({'dateFormat:tz': 'UTC', });
});
after(async () => {
@@ -28,6 +32,7 @@ export default function ({ getService, loadTestFile }: FtrProviderContext) {
await kibanaServer.importExport.unload(
'x-pack/test/functional/fixtures/kbn_archiver/lens/lens_basic.json'
);
+ await PageObjects.timePicker.resetDefaultAbsoluteRangeViaUiSettings();
});
describe('', function () {
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.
also note that a couple of lens tests load other esArchives, which wipes out the defaultTime setting change made in the lens/index file. So if you wanted to remove the calls to the timepicker in those tests, you would have to add the call to
await PageObjects.timePicker.setDefaultAbsoluteRangeViaUiSettings();
in those tests after the esArchive load.
await esArchiver.unload('x-pack/test/functional/es_archives/lens/basic'); | ||
await kibanaServer.importExport.unload( | ||
'x-pack/test/functional/fixtures/kbn_archiver/lens/lens_basic.json' | ||
); |
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.
As noted in the comment above, make sure to reset the default time in the after method;
await PageObjects.timePicker.resetDefaultAbsoluteRangeViaUiSettings();
6f757aa
to
fef00ed
Compare
@elasticmachine merge upstream |
e28ecd6
to
e0a489d
Compare
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@@ -56,17 +57,19 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { | |||
}); | |||
|
|||
it('should allow to configure column visibility', async () => { | |||
screenshot.take('toggleColumnVisibility0: before'); |
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.
these and the x-pack/test/functional/page_objects/lens_page.ts screenshots are to be removed right? I think I left it there 🙈
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.
aww we were happy about them. Thank you again for all that you did for this maddening frustrating pr @mbondyra you rock!
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've removed the screenshots I've added before so make sure pull the changes :) This is an enormous effort and thank you for it @bhavyarm, I have no remarks about the code and I'll approve as soon as the CI tests pass 🤞🏼
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.
LGTM
Looks like flaky test runner doesn't work as expected not only for your branch but even for main. So your PR looks good to me! Approved :) |
One more thing - please be cautious when merging it to 7.16 - there is one unmerged PR in the past that can cause conflicts here. The mentioned PR was causing failures for 7.16 even though the change seems irrelevant (changing CI groups to balance functional tests) and that's why it wasn't merged to 7.16. There's a chance this PR can cause the same so let's keep an eye on it. :) |
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @bhavyarm |
💔 Backport failed
Successful backport PRs will be merged automatically after passing CI. To backport manually run: |
# Conflicts: # x-pack/test/api_integration/apis/lens/telemetry.ts # x-pack/test/functional/es_archives/lens/basic/mappings.json
This is part 1 of migrating lens .kibana esArchiver to kbnArchiver.