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.

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

Fix ScopedHistory mock and adapt usages #71404

Merged

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Jul 13, 2020

Summary

Fix #71166

  • Make the ScopedHistoryMock type actually extends ScopedHistory
  • Adapt test usages and remove (now) unnecessary casting

Comment on lines 54 to 55
return mock;
return mock as ScopedHistoryMock;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I thought about creating a IScopedHistory interface, however the concrete ScopedHistory type is widely used in the codebase, so it was causing a lot of changes, which is why I decided to force cast to jest.Mocked<ScopedHistory> instead.

I think this is fine because:

  • this is only used for testing
  • we are still ensuring that the mock implements all public methods/properties of ScopedHistory with the const mock: jest.Mocked<Pick<ScopedHistory, keyof ScopedHistory>> declaration, so any changes in ScopedHistory public API would cause a TS error until the mock construction is adapted.
  • only private methods and properties are actually missing from the returned mock instance.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a comment on why the casting is necessary?

@pgayvallet pgayvallet added release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.9.0 v8.0.0 labels Jul 13, 2020
@pgayvallet pgayvallet marked this pull request as ready for review July 13, 2020 08:32
@pgayvallet pgayvallet requested review from a team as code owners July 13, 2020 08:32
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@botelastic botelastic bot added the Feature:Embedding Embedding content via iFrame label Jul 13, 2020
Comment on lines 54 to 55
return mock;
return mock as ScopedHistoryMock;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a comment on why the casting is necessary?

Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

Security/Spaces code LGTM, thanks!

Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

Alerting changes LGTM

Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

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

AppArch changes LGTM

Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

ES UI changes LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

@pgayvallet pgayvallet merged commit f0fe9b1 into elastic:master Jul 14, 2020
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Jul 14, 2020
* Fix mock and adapt usages

* fix snapshots

* add comment about forcecast

* remove mock overrides
pgayvallet added a commit that referenced this pull request Jul 14, 2020
* Fix mock and adapt usages

* fix snapshots

* add comment about forcecast

* remove mock overrides
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 14, 2020
* master: (314 commits)
  [APM] Use status_code field to calculate error rate (elastic#71109)
  [Observability] Change appLink passing the date range (elastic#71259)
  [Security] Add Timeline improvements (elastic#71506)
  adjust vislib bar opacity (elastic#71421)
  Fix ScopedHistory mock and adapt usages (elastic#71404)
  [Security Solution] Add hook for reading/writing resolver query params (elastic#70809)
  [APM] Bug fixes from ML integration testing (elastic#71564)
  [Discover] Add caused_by.type and caused_by.reason to error toast modal (elastic#70404)
  [Security Solution] Add 3rd level breadcrumb to admin page (elastic#71275)
  [Security Solution][Exceptions] Exception modal bulk close alerts that match exception attributes (elastic#71321)
  Change signal.rule.risk score mapping from keyword to float (elastic#71126)
  Added help text where needed on connectors and alert actions UI (elastic#69601)
  [SIEM][Detections] Value Lists Management Modal (elastic#67068)
  [test] Skips test preventing promotion of ES snapshot elastic#71582
  [test] Skips test preventing promotion of ES snapshot elastic#71555
  [ILM] Fix alignment of the timing field (elastic#71273)
  [SIEM][Detection Engine][Lists] Adds the ability for exception lists to be multi-list queried. (elastic#71540)
  initial telemetry setup (elastic#69330)
  [Reporting] Formatting fixes for CSV export in Discover, CSV download from Dashboard panel (elastic#67027)
  Search across spaces (elastic#67644)
  ...
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jul 14, 2020
…t-apps-page-titles

* 'master' of github.com:elastic/kibana: (88 commits)
  [ML] Functional tests - disable DFA creation and cloning tests
  [APM] Use status_code field to calculate error rate (elastic#71109)
  [Observability] Change appLink passing the date range (elastic#71259)
  [Security] Add Timeline improvements (elastic#71506)
  adjust vislib bar opacity (elastic#71421)
  Fix ScopedHistory mock and adapt usages (elastic#71404)
  [Security Solution] Add hook for reading/writing resolver query params (elastic#70809)
  [APM] Bug fixes from ML integration testing (elastic#71564)
  [Discover] Add caused_by.type and caused_by.reason to error toast modal (elastic#70404)
  [Security Solution] Add 3rd level breadcrumb to admin page (elastic#71275)
  [Security Solution][Exceptions] Exception modal bulk close alerts that match exception attributes (elastic#71321)
  Change signal.rule.risk score mapping from keyword to float (elastic#71126)
  Added help text where needed on connectors and alert actions UI (elastic#69601)
  [SIEM][Detections] Value Lists Management Modal (elastic#67068)
  [test] Skips test preventing promotion of ES snapshot elastic#71582
  [test] Skips test preventing promotion of ES snapshot elastic#71555
  [ILM] Fix alignment of the timing field (elastic#71273)
  [SIEM][Detection Engine][Lists] Adds the ability for exception lists to be multi-list queried. (elastic#71540)
  initial telemetry setup (elastic#69330)
  [Reporting] Formatting fixes for CSV export in Discover, CSV download from Dashboard panel (elastic#67027)
  ...

# Conflicts:
#	x-pack/plugins/index_management/public/application/index.tsx
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 14, 2020
* master: (72 commits)
  [test] Skips test preventing promotion of ES snapshot elastic#71612
  [Logs UI] Remove UUID from Alert Instances (elastic#71340)
  [Metrics UI] Remove UUID from Alert Instance IDs (elastic#71335)
  [ML] Functional tests - disable DFA creation and cloning tests
  [APM] Use status_code field to calculate error rate (elastic#71109)
  [Observability] Change appLink passing the date range (elastic#71259)
  [Security] Add Timeline improvements (elastic#71506)
  adjust vislib bar opacity (elastic#71421)
  Fix ScopedHistory mock and adapt usages (elastic#71404)
  [Security Solution] Add hook for reading/writing resolver query params (elastic#70809)
  [APM] Bug fixes from ML integration testing (elastic#71564)
  [Discover] Add caused_by.type and caused_by.reason to error toast modal (elastic#70404)
  [Security Solution] Add 3rd level breadcrumb to admin page (elastic#71275)
  [Security Solution][Exceptions] Exception modal bulk close alerts that match exception attributes (elastic#71321)
  Change signal.rule.risk score mapping from keyword to float (elastic#71126)
  Added help text where needed on connectors and alert actions UI (elastic#69601)
  [SIEM][Detections] Value Lists Management Modal (elastic#67068)
  [test] Skips test preventing promotion of ES snapshot elastic#71582
  [test] Skips test preventing promotion of ES snapshot elastic#71555
  [ILM] Fix alignment of the timing field (elastic#71273)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Embedding Embedding content via iFrame release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make ScopedHistoryMock compatible with ScopedHistory
8 participants