-
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
[Reporting] Remove ReportingCore references from server/export_types shared code #158031
Conversation
Pinging @elastic/appex-sharedux (Team:SharedUX) |
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.
Requesting changes on the way mocks are provided in the unit tests.
Otherwise, this is starting to look great!
@@ -34,8 +34,9 @@ test(`gets logo from uiSettings`, async () => { | |||
mockReportingPlugin.getUiSettingsServiceFactory = jest.fn().mockResolvedValue({ | |||
get: mockGet, | |||
}); | |||
|
|||
const { logo } = await getCustomLogo(mockReportingPlugin, headers, 'spaceyMcSpaceIdFace', logger); | |||
const fakeRequest = mockReportingPlugin.getFakeRequest(headers, 'spaceyMcSpaceIdFace', logger); |
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.
We will need to take mock reporting core usages out of the unit tests in x-pack/plugins/reporting/server/export_types
.
For the fake request:
import { httpServerMock } from '@kbn/core-http-server-mocks';
const mockRequest = httpServerMock.createKibanaRequest();
For the UI Settings client:
import { uiSettingsServiceMock } from '@kbn/core/public/mocks';
const mockUiSettings = uiSettingsServiceMock.createStartContract();
edit: sorry for the bad example -- uiSettingsServiceMock can't be imported from public
in this code
@@ -30,7 +30,8 @@ beforeEach(async () => { | |||
const getMockJob = (base: object) => base as TaskPayloadPNG & TaskPayloadPDF; | |||
|
|||
test(`fails if no URL is passed`, async () => { | |||
const fn = () => getFullUrls(mockReporting, getMockJob({})); | |||
const fn = () => | |||
getFullUrls(mockReporting.getServerInfo(), mockReporting.getConfig(), getMockJob({})); |
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.
Same as above: the server info and config should be mocked without a mockReportingCore
expect( | ||
getFullRedirectAppUrl( | ||
reporting.getConfig(), | ||
reporting.getServerInfo(), |
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.
Same as above: the server info and config should be mocked without a mockReportingCore
return lastValueFrom( | ||
generatePngObservable(reporting, logger, { |
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.
😎
return generatePdfObservable(reporting, { | ||
// make a new function that will call reporting.getScreenshots | ||
const snapshotFn = () => | ||
reporting.getScreenshots({ |
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.
It's OK to leave reportingCore in the execute_job
/ create_job
functions for now. We'll sweep through these in the next PR.
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 just wanted to make sure I wasn't misinterpreting this - so how this file and others are is ok, right? I had refactored the generatePdfObservable() to take in the snapshot function vs all of reporting core.
Eventually I will remove any call to reporting.getScreenshots() etc in this file and others in future PRs :yay: but for this PR, this can stay as it is right?
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.
Yes, these files called x-pack/plugins/reporting/server/export_types/*/execute_job*
as well as x-pack/plugins/reporting/server/export_types/csv_v2/create_job.ts
are OK, and how I would expect them to be at this stage of our progress.
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.
Ok thank you!!
💚 Build Succeeded
Metrics [docs]Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @rshen91 |
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
reviewed the code
## Summary Part of elastic#158092 Removing calls to `ReportingCore` in export types functions ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
Summary
Part of #158092
Removing calls to
ReportingCore
in export types functionsChecklist