Skip to content

[Reporting] Consolidate startup self-checks - #31931

Merged
tsullivan merged 11 commits into
elastic:masterfrom
tsullivan:enhance/reporting-init-check-url
Mar 14, 2019
Merged

[Reporting] Consolidate startup self-checks#31931
tsullivan merged 11 commits into
elastic:masterfrom
tsullivan:enhance/reporting-init-check-url

Conversation

@tsullivan

@tsullivan tsullivan commented Feb 25, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Summary

Edits from investigating #31856

Maintenance/Cleanup:

  • Move get_absolute_url to a common libs folder
  • Move all the validate utilities to a dedicated folder
  • Pass a logger object with multiple methods, instead of a single function bound to a single log tag set.
  • Remove try/catch from x-pack/plugins/reporting/server/lib/validate_max_content_length.ts as error catching is handled at a higher level

@tsullivan tsullivan added release_note:enhancement zDeprecated Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead v8.0.0 v7.2.0 review labels Feb 25, 2019
tsullivan
tsullivan commented Feb 25, 2019
View reviewed changes
Comment thread x-pack/plugins/reporting/server/lib/validate/validate_browser.ts Outdated
const page = await browser.newPage();
const url = getAbsoluteUrl({ path: API_STATS_ENDPOINT });
logger.debug(`Opening page ${url}`);
await page.goto(url, { waitUntil: 'networkidle0' }); // Look for JSON response

@tsullivan tsullivan Feb 25, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Other than the various moving files around and changing a few interfaces, this is the "key" change

@joelgriffith joelgriffith Feb 25, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sweet. I was just thinking it'd be a good idea to expand this test and make sure we can navigate. Glad you beat me to it!

joelgriffith
joelgriffith reviewed Feb 25, 2019
View reviewed changes
Comment thread x-pack/plugins/reporting/server/lib/validate/index.ts
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

@joelgriffith joelgriffith Feb 25, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice, I like this!

joelgriffith
joelgriffith approved these changes Feb 25, 2019
View reviewed changes

@joelgriffith joelgriffith left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great. We should do a follow up issue at some point to remove all the browser type checks since we've deprecated phantom. Not related to this PR, just thinking out loud here.

@tsullivan

tsullivan commented Feb 26, 2019

Copy link
Copy Markdown
Member Author

Looks great. We should do a follow up issue at some point to remove all the browser type checks since we've deprecated phantom. Not related to this PR, just thinking out loud here.

Yes! There is an existing issue on this: #27136

@elastic elastic deleted a comment from elasticmachine Feb 26, 2019
@elastic elastic deleted a comment from elasticmachine Feb 26, 2019
@elastic elastic deleted a comment from elasticmachine Feb 27, 2019
@elastic elastic deleted a comment from elasticmachine Feb 27, 2019
@elastic elastic deleted a comment from elasticmachine Feb 27, 2019
@tsullivan

tsullivan commented Feb 27, 2019

Copy link
Copy Markdown
Member Author

retest

@tsullivan

tsullivan commented Feb 28, 2019

Copy link
Copy Markdown
Member Author

Changes in this PR might be causing this failure in CI:

16:05:51          │ info [es] stopped
16:05:51  PASS  test_utils/jest/integration_tests/example_integration.test.ts (92.234s)
16:05:51   example integration test with kbn server
16:05:51     ✓ should have started new platform server correctly (3ms)
16:05:51 
16:05:51 Test Suites: 1 passed, 1 total
16:05:51 Tests:       1 passed, 1 total
16:05:51 Snapshots:   0 total
16:05:51 Time:        93.567s
16:05:51 Ran all test suites.
16:05:51          │ info [es] cleanup complete
16:05:52 Jest did not exit one second after the test run has completed.
16:05:52 
16:05:52 This usually means that there are asynchronous operations that weren't stopped in your tests. Consider running Jest with `--detectOpenHandles` to troubleshoot this issue.

@elasticmachine

elasticmachine commented Feb 28, 2019

Copy link
Copy Markdown
Contributor

💔 Build Failed

@tsullivan

tsullivan commented Feb 28, 2019

Copy link
Copy Markdown
Member Author

I think this is slowing down Kibana server startup, and CI is failing due to timeouts

@elasticmachine

elasticmachine commented Mar 4, 2019

Copy link
Copy Markdown
Contributor

💔 Build Failed

@tsullivan

tsullivan commented Mar 12, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

I'd like to get this in because of the cleanup changes. For now, I removed the part this added that opens a Kibana URL as a self-test check.

That part would be nice to have, if it worked. Another plan could be to schedule a one-off task in Task Manager that does this, to be sure we're not fighting for startup resources when we do the self-check.

@elasticmachine

elasticmachine commented Mar 12, 2019

Copy link
Copy Markdown
Contributor

💚 Build Succeeded

@tsullivan tsullivan changed the title [Reporting] Open test page in reporting browser self-check [Reporting] Consolidate startup self-checks Mar 14, 2019
@tsullivan
tsullivan merged commit 4309a8b into elastic:master Mar 14, 2019
@tsullivan
tsullivan deleted the enhance/reporting-init-check-url branch March 14, 2019 00:28
tsullivan added a commit to tsullivan/kibana that referenced this pull request Mar 14, 2019
@tsullivan
[Reporting] Consolidate startup self-checks (elastic#31931)
a838a95 
* [Reporting] Open test page in reporting browser self-check

* comment correction

* fix tests

* fix test

* fix tests more

* remove test of open Kibana URL
@tsullivan tsullivan mentioned this pull request Mar 14, 2019
Merged
tsullivan added a commit that referenced this pull request Mar 14, 2019
@tsullivan
[Reporting] Consolidate startup self-checks (#31931) (#33213)
87357b3 
* [Reporting] Open test page in reporting browser self-check

* comment correction

* fix tests

* fix test

* fix tests more

* remove test of open Kibana URL
patrykkopycinski pushed a commit to patrykkopycinski/kibana that referenced this pull request May 6, 2026
@tsullivan
[Reporting] Consolidate startup self-checks (elastic#31931)
9fe8144 
* [Reporting] Open test page in reporting browser self-check

* comment correction

* fix tests

* fix test

* fix tests more

* remove test of open Kibana URL
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stacey-gammon stacey-gammon Awaiting requested review from stacey-gammon
@timroes timroes Awaiting requested review from timroes
1 more reviewer
@joelgriffith joelgriffith joelgriffith approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

review v7.2.0 v8.0.0 zDeprecated Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tsullivan @elasticmachine @joelgriffith