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.

Sign up for GitHub

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

Jump to bottom

[code coverage] adding plugin to flush coverage data #83447

Merged
merged 5 commits into from
Nov 18, 2020
Merged

[code coverage] adding plugin to flush coverage data #83447

merged 5 commits into from
Nov 18, 2020

Conversation

dmlemeshko
Copy link
Member

@dmlemeshko dmlemeshko commented Nov 16, 2020 •
edited
Loading

Summary

In order to bring back code coverage collection for functional tests this PR adds new plugin that simply adds flushCoverageToLog that we use to check for coverage before unload event and put it into console, in case __coverage__ property has data.

Shout-out to @restrry for plugin idea!

Related code coverage job to test PR: https://kibana-ci.elastic.co/job/elastic+kibana+qa-research/131/

@apmmachine
Copy link
Contributor

💚 Build Succeeded

@dmlemeshko dmlemeshko added 8.0.0 backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes labels Nov 16, 2020
@spalger spalger added v8.0.0 and removed 8.0.0 labels Nov 16, 2020
@dmlemeshko dmlemeshko marked this pull request as ready for review November 16, 2020 19:21
wayneseymour
wayneseymour reviewed Nov 16, 2020
View reviewed changes
test/common/fixtures/plugins/coverage/public/plugin.ts Outdated
public start() {}

public setup() {
(window as WindowWithCoverage & typeof globalThis).flushCoverageToLog = function () {
Copy link
Member

Choose a reason for hiding this comment

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

is that not supposed to be double-and &&?

Copy link
Member

Choose a reason for hiding this comment

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

Single-and is a bitwise op.

Copy link
Member Author

@dmlemeshko dmlemeshko Nov 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think it is needed here, since I'm not doing logical operation like window && typeof window === "object"
I just searched a bit and found it here

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, in type signatures a single & between two types creates a union type of the two.

wayneseymour
wayneseymour requested changes Nov 16, 2020
View reviewed changes
Copy link
Member

@wayneseymour wayneseymour left a comment

Choose a reason for hiding this comment

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

Just concerned about the bitwise ops.

spalger
spalger reviewed Nov 16, 2020
View reviewed changes
test/common/fixtures/plugins/coverage/public/plugin.ts Outdated
public start() {}

public setup() {
(window as WindowWithCoverage & typeof globalThis).flushCoverageToLog = function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can clean this up by creating a local variable with the types assigned once and use that local variable rather than re-casting window over and over.

Copy link
Member Author

Choose a reason for hiding this comment

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

@spalger @wayneseymour what do you think about 38163d2

declare global {
  interface Window {
    __coverage__: any;
    flushCoverageToLog: any;
  }
}

spalger
spalger approved these changes Nov 16, 2020
View reviewed changes
Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

LGTM

@dmlemeshko
Copy link
Member Author

Many tests failed with the same error:

[00:03:10]               │ debg browser[INFO] http://localhost:6111/bootstrap.js 42:19 "^ A single error about an inline script not firing due to content security policy is expected!"
[00:03:10]               │ERROR browser[SEVERE] http://localhost:6111/9007199254740991/bundles/plugin/coverage-fixtures/coverage-fixtures.plugin.js - Failed to load resource: the server responded with a status of 404 (Not Found)

That was not a case when I updated bootstrap.js template directly. @restrry did I miss something important?

@mshustov
Copy link
Contributor

@dmlemeshko coverage-fixtures plugin doesn't seem to have been built. AFAIK You need to add it here

kibana/test/scripts/jenkins_build_plugins.sh

Line 8 in f5ca7d8

--scan-dir "$KIBANA_DIR/test/plugin_functional/plugins" \

kibana/test/scripts/jenkins_xpack_build_plugins.sh

Line 7 in a8c080b

--scan-dir "$KIBANA_DIR/test/plugin_functional/plugins" \

@elastic/kibana-operations is that correct?

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@dmlemeshko
Copy link
Member Author

All good now, coverage is fixed
Thank you @restrry

@dmlemeshko dmlemeshko merged commit 27125bc into elastic:master Nov 18, 2020
phillipb added a commit to phillipb/kibana that referenced this pull request Nov 18, 2020
@phillipb
Merge branch 'master' of github.com:elastic/kibana into add-metrics-t…
2def566 
…o-node-details

* 'master' of github.com:elastic/kibana: (65 commits)
  update chromedriver dependency to 87 (elastic#83624)
  [TSVB] use new Search API for rollup search (elastic#83275)
  [TSVB] Y-axis has number formatting not considering all series formatters in the group (elastic#83438)
  [Logs UI] Update <LogStream /> internal state when its props change (elastic#83302)
  Add tag bulk action context menu (elastic#82816)
  [code coverage] adding plugin to flush coverage data (elastic#83447)
  [UsageCollection] Expose `KibanaRequest` to explicitly opted-in collectors (elastic#83413)
  Added eventBus to trigger and listen plotHandler event (elastic#83435)
  [Runtime fields] Editor phase 1 (elastic#81472)
  [Maps] Fix threshold alert issue resolving nested fields (elastic#83577)
  chore(NA): remove usage of unverified es snapshots (elastic#83589)
  [DOCS] Adds Elastic Contributor Program link (elastic#83561)
  Upgrade EUI to v30.2.0 (elastic#82730)
  Don't show loading screen during auto-reload (elastic#83376)
  Functional tests - fix esArchive mappings with runtime fields (elastic#83530)
  [deb/rpm] Create keystore after installation (elastic#76465)
  [rpm] Create default environment file at "/etc/sysconfig/kibana" (elastic#82144)
  [docker] removes workaround for missing crypto-policies-scripts subpackage (elastic#83455)
  [ML] Persisted URL state for the Data frame analytics jobs and models pages (elastic#83439)
  adds xpack.security.authc.selector.enabled setting (elastic#83551)
  ...
@dmlemeshko dmlemeshko deleted the coverage-plugin branch January 31, 2022 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wayneseymour wayneseymour wayneseymour approved these changes

@spalger spalger spalger approved these changes

@mshustov mshustov Awaiting requested review from mshustov

@LeeDr LeeDr Awaiting requested review from LeeDr

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dmlemeshko @apmmachine @mshustov @kibanamachine @spalger @wayneseymour