-
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
[Application Usage] Functional test to validate the full list of appIds in the schema #88080
[Application Usage] Functional test to validate the full list of appIds in the schema #88080
Conversation
@@ -0,0 +1,44 @@ | |||
/* |
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 think that this new folder of functional tests can be used as well when implementing #83704
04f3052
to
5f41295
Compare
@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.
Just be sure the test suite is completely set up for CI correctly, have we tested making pushing a failing version of the test to CI? Sometimes it's easy to miss a detail about adding a new functional suite and I like to use that as a gut-check.
try { | ||
// When the lists don't match, the entire page load will fail. | ||
expect(Object.keys(applicationUsageSchema).sort()).to.eql(appIds); | ||
} catch (err) { | ||
err.message = `Application Usage's schema is not up-to-date with the actual registered apps. Please update it at src/plugins/kibana_usage_collection/server/collectors/application_usage/schema.ts.\n${err.message}`; | ||
throw err; | ||
} |
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.
Instead of throwing an error on startup, it may be nicer to have this plugin set a property on window.__usageCollectionTest__
or similar and have the test read that property. That way, other tests that we add to this suite can still run even if this test is currently failing.
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.
That makes perfect sense! In fact, I had a thought about it, but couldn't find a way to read the window
object in the tests and thought it was not possible. Now that you mentioned, I ran a second search and found the right way to do it. Thanks! I'll push the changes in a bit.
Re the CI, I've checked Jenkins' logs to make sure the test runs. And I tried locally the fail scenario in the test. I'll push a failed scenario anyway to ensure we are setting things right by seeing the CI fail 👍
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 CI failed as expected: https://kibana-ci.elastic.co/job/elastic+kibana+pipeline-pull-request/99129/testReport/ 🎉
Running a last build with the correct schema.
@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.
LGTM
// These keys obtained by searching for `/application\w*\.register\(/` and checking the value of the attr `id`. | ||
// TODO: Find a way to update these keys automatically. | ||
// There is a test in x-pack/test/usage_collection that validates that the keys in here match all the registered apps |
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 adding a new app will cause the test to fail, maybe there should be a comment on the test pointing to this file to help developers fix it when they'll add new apps?
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.
@pgayvallet makes sense! I've updated the test to point at the file in the error message.
Here's an example of the output:
0 passing (10.4s)
│1 failing
│
│1) Application Usage
│ keys in the schema match the registered application IDs:
│
│
│ Error: Application Usage's schema is not up-to-date with the actual registered apps. Please update it at src/plugins/kibana_usage_collection/server/collectors/application_usage/schema.ts.
│ expected [ 'apm',
│ 'appSearch',
│ 'canvas',
│ 'dashboard_mode',
│ 'dashboards',
│ 'dev_tools',
│ 'discover',
│ 'enterpriseSearch',
│ 'error',
│ 'fleet',
│ 'graph',
│ 'home',
│ 'infra',
│ 'ingestManager',
│ 'kibana',
│ 'kibanaOverview',
│ 'lens',
│ 'logs',
│ 'management',
│ 'maps',
│ 'metrics',
│ 'ml',
│ 'monitoring',
│ 'observability-overview',
│ 'securitySolution',
│ 'securitySolution:administration',
│ 'securitySolution:case',
│ 'securitySolution:detections',
│ 'securitySolution:hosts',
│ 'securitySolution:network',
│ 'securitySolution:overview',
│ 'securitySolution:timelines',
│ 'security_access_agreement',
│ 'security_account',
│ 'security_capture_url',
│ 'security_logged_out',
│ 'security_login',
│ 'security_logout',
│ 'security_overwritten_session',
│ 'short_url_redirect',
│ 'siem',
│ 'space_selector',
│ 'status',
│ 'test',
│ 'timelion',
│ 'uptime',
│ 'ux',
│ 'visualize',
│ 'workplaceSearch' ] to sort of equal [ 'apm',
│ 'appSearch',
│ 'canvas',
│ 'dashboard_mode',
│ 'dashboards',
│ 'dev_tools',
│ 'discover',
│ 'enterpriseSearch',
│ 'error',
│ 'fleet',
│ 'graph',
│ 'home',
│ 'infra',
│ 'ingestManager',
│ 'kibana',
│ 'kibanaOverview',
│ 'lens',
│ 'logs',
│ 'management',
│ 'maps',
│ 'metrics',
│ 'ml',
│ 'monitoring',
│ 'observability-overview',
│ 'securitySolution',
│ 'securitySolution:administration',
│ 'securitySolution:case',
│ 'securitySolution:detections',
│ 'securitySolution:hosts',
│ 'securitySolution:network',
│ 'securitySolution:overview',
│ 'securitySolution:timelines',
│ 'security_access_agreement',
│ 'security_account',
│ 'security_capture_url',
│ 'security_logged_out',
│ 'security_login',
│ 'security_logout',
│ 'security_overwritten_session',
│ 'short_url_redirect',
│ 'siem',
│ 'space_selector',
│ 'status',
│ 'timelion',
│ 'uptime',
│ 'ux',
│ 'visualize',
│ 'workplaceSearch' ]
│ + expected - actual
│
│ "short_url_redirect"
│ "siem"
│ "space_selector"
│ "status"
│ - "test"
│ "timelion"
│ "uptime"
│ "ux"
│ "visualize"
│
│ at Assertion.assert (/Users/afharo/Developer/elastic/kibana/packages/kbn-expect/expect.js:100:11)
│ at Assertion.eql (/Users/afharo/Developer/elastic/kibana/packages/kbn-expect/expect.js:244:8)
│ at Context.<anonymous> (test/usage_collection/test_suites/application_usage/index.ts:21:63)
│ at runMicrotasks (<anonymous>)
│ at processTicksAndRejections (internal/process/task_queues.js:93:5)
│ at Object.apply (/Users/afharo/Developer/elastic/kibana/packages/kbn-test/src/functional_test_runner/lib/mocha/wrap_function.js:84:16)
│
│
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! thanks.
…sage/functiona-test-appIds-in-schema
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
Summary
This PR adds functional tests to the Application Usage schema definition: it runs Kibana and checks that all the full list of registered apps is detailed in the schema (all appIds should be set as keys in the applicationUsage schema).
Resolves #75329
Checklist
Delete any items that are not applicable to this PR.
For maintainers