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

[APM] Prevent console.error causing unit tests to fail locally #161959

Merged
merged 2 commits into from
Jul 18, 2023
Merged

[APM] Prevent console.error causing unit tests to fail locally #161959

merged 2 commits into from
Jul 18, 2023

Conversation

sorenlouv
Copy link
Member

@sorenlouv sorenlouv commented Jul 14, 2023 •
edited by kibanamachine
Loading

#161636 fixed a couple of unit tests that were failing locally but passing on CI. This PR should prevent this from happening again.

Why they failed locally and not on CI??
Locally console.error is treated as a test failure:

kibana/x-pack/plugins/apm/jest_setup.js

Lines 12 to 15 in 7ea0dd6

jest.spyOn(console, 'error').mockImplementation((message, ...args) => {
console.log(message, ...args);
throw new Error(message);
});

Whereas on CI console.* is disabled:

kibana/packages/kbn-test/src/jest/setup/disable_console_logs.js

Lines 9 to 12 in a78c7b0

// on CI these logs just muddy up the console and produce a ton of unnecessary noise
console.log = () => {};
console.error = () => {};
console.warn = () => {};

This means that if a test logs console.error it would fail locally but not on CI. This PR changes that so console.error will not cause unit tests to fail anywhere.

@sorenlouv sorenlouv requested a review from a team as a code owner July 14, 2023 12:31
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Jul 14, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:APM)

@apmmachine
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@sorenlouv sorenlouv added v8.9.0 release_note:skip Skip the PR/issue when compiling release notes labels Jul 14, 2023
@sorenlouv sorenlouv enabled auto-merge (squash) July 14, 2023 12:31
achyutjhunjhunwala
achyutjhunjhunwala approved these changes Jul 17, 2023
View reviewed changes
Copy link
Contributor

@achyutjhunjhunwala achyutjhunjhunwala left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for fixing this

gbamparop
gbamparop approved these changes Jul 17, 2023
View reviewed changes
Copy link
Contributor

@gbamparop gbamparop left a comment

Choose a reason for hiding this comment

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

LGTM!

@sorenlouv sorenlouv force-pushed the prevent-console-errors-from-failing-tests branch from 38e55cd to 23857d8 Compare July 17, 2023 14:45
@sorenlouv sorenlouv force-pushed the prevent-console-errors-from-failing-tests branch from 23857d8 to 599cab0 Compare July 17, 2023 23:13
@sorenlouv sorenlouv merged commit c842479 into elastic:main Jul 18, 2023
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

✅ unchanged

History

  • 💔 Build #142917 failed 23857d81bfbe6dc2dc5be3e568aae2cbe03d9680
  • 💔 Build #142825 failed 38e55cd8c1c2217d974a375aad979fccf2eb21c3
  • 💛 Build #142528 was flaky 3f2bcf3dfec16e03a4b33266329ca9608a28977b

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

@kibanamachine kibanamachine mentioned this pull request Jul 18, 2023
Merged
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jul 18, 2023
@sorenlouv
[APM] Prevent console.error causing unit tests to fail locally (elast…
9a0b989 
…ic#161959)

elastic#161636 fixed a couple of unit
tests that were failing locally but passing on CI. This PR should
prevent this from happening again.

**Why they failed locally and not on CI??**
Locally `console.error` is treated as a test failure:

https://github.com/elastic/kibana/blob/7ea0dd6b116a93024d68ea2d93fa4ce90e9bf189/x-pack/plugins/apm/jest_setup.js#L12-L15

Whereas on CI `console.*` is disabled:

https://github.com/elastic/kibana/blob/a78c7b02b3b825826f39289e91e545ee6f4a67d9/packages/kbn-test/src/jest/setup/disable_console_logs.js#L9-L12

This means that if a test logs `console.error` it would fail locally but
not on CI. This PR changes that so console.error will not cause unit
tests to fail anywhere.

(cherry picked from commit c842479)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.9

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Jul 18, 2023
@kibanamachine @sorenlouv
[8.9] [APM] Prevent console.error causing unit tests to fail locally (#…
acb55e9 
…161959) (#162100)

# Backport

This will backport the following commits from `main` to `8.9`:
- [[APM] Prevent console.error causing unit tests to fail locally
(#161959)](#161959)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Søren
Louv-Jansen","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-07-18T00:45:58Z","message":"[APM]
Prevent console.error causing unit tests to fail locally
(#161959)\n\nhttps://github.com//pull/161636 fixed a
couple of unit\r\ntests that were failing locally but passing on CI.
This PR should\r\nprevent this from happening again.\r\n\r\n**Why they
failed locally and not on CI??**\r\nLocally `console.error` is treated
as a test
failure:\r\n\r\nhttps://github.com/elastic/kibana/blob/7ea0dd6b116a93024d68ea2d93fa4ce90e9bf189/x-pack/plugins/apm/jest_setup.js#L12-L15\r\n\r\nWhereas
on CI `console.*` is
disabled:\r\n\r\nhttps://github.com/elastic/kibana/blob/a78c7b02b3b825826f39289e91e545ee6f4a67d9/packages/kbn-test/src/jest/setup/disable_console_logs.js#L9-L12\r\n\r\nThis
means that if a test logs `console.error` it would fail locally
but\r\nnot on CI. This PR changes that so console.error will not cause
unit\r\ntests to fail
anywhere.","sha":"c842479d633ee28ddda3ce960e1341f20d5801e3","branchLabelMapping":{"^v8.10.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:APM","release_note:skip","v8.9.0","v8.10.0"],"number":161959,"url":"https://github.com/elastic/kibana/pull/161959","mergeCommit":{"message":"[APM]
Prevent console.error causing unit tests to fail locally
(#161959)\n\nhttps://github.com//pull/161636 fixed a
couple of unit\r\ntests that were failing locally but passing on CI.
This PR should\r\nprevent this from happening again.\r\n\r\n**Why they
failed locally and not on CI??**\r\nLocally `console.error` is treated
as a test
failure:\r\n\r\nhttps://github.com/elastic/kibana/blob/7ea0dd6b116a93024d68ea2d93fa4ce90e9bf189/x-pack/plugins/apm/jest_setup.js#L12-L15\r\n\r\nWhereas
on CI `console.*` is
disabled:\r\n\r\nhttps://github.com/elastic/kibana/blob/a78c7b02b3b825826f39289e91e545ee6f4a67d9/packages/kbn-test/src/jest/setup/disable_console_logs.js#L9-L12\r\n\r\nThis
means that if a test logs `console.error` it would fail locally
but\r\nnot on CI. This PR changes that so console.error will not cause
unit\r\ntests to fail
anywhere.","sha":"c842479d633ee28ddda3ce960e1341f20d5801e3"}},"sourceBranch":"main","suggestedTargetBranches":["8.9"],"targetPullRequestStates":[{"branch":"8.9","label":"v8.9.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.10.0","labelRegex":"^v8.10.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/161959","number":161959,"mergeCommit":{"message":"[APM]
Prevent console.error causing unit tests to fail locally
(#161959)\n\nhttps://github.com//pull/161636 fixed a
couple of unit\r\ntests that were failing locally but passing on CI.
This PR should\r\nprevent this from happening again.\r\n\r\n**Why they
failed locally and not on CI??**\r\nLocally `console.error` is treated
as a test
failure:\r\n\r\nhttps://github.com/elastic/kibana/blob/7ea0dd6b116a93024d68ea2d93fa4ce90e9bf189/x-pack/plugins/apm/jest_setup.js#L12-L15\r\n\r\nWhereas
on CI `console.*` is
disabled:\r\n\r\nhttps://github.com/elastic/kibana/blob/a78c7b02b3b825826f39289e91e545ee6f4a67d9/packages/kbn-test/src/jest/setup/disable_console_logs.js#L9-L12\r\n\r\nThis
means that if a test logs `console.error` it would fail locally
but\r\nnot on CI. This PR changes that so console.error will not cause
unit\r\ntests to fail
anywhere.","sha":"c842479d633ee28ddda3ce960e1341f20d5801e3"}}]}]
BACKPORT-->

Co-authored-by: Søren Louv-Jansen <[email protected]>
ThomThomson pushed a commit to ThomThomson/kibana that referenced this pull request Aug 1, 2023
@sorenlouv
[APM] Prevent console.error causing unit tests to fail locally (elast…
38ff66c 
…ic#161959)

elastic#161636 fixed a couple of unit
tests that were failing locally but passing on CI. This PR should
prevent this from happening again.

**Why they failed locally and not on CI??**
Locally `console.error` is treated as a test failure:

https://github.com/elastic/kibana/blob/7ea0dd6b116a93024d68ea2d93fa4ce90e9bf189/x-pack/plugins/apm/jest_setup.js#L12-L15

Whereas on CI `console.*` is disabled:

https://github.com/elastic/kibana/blob/a78c7b02b3b825826f39289e91e545ee6f4a67d9/packages/kbn-test/src/jest/setup/disable_console_logs.js#L9-L12

This means that if a test logs `console.error` it would fail locally but
not on CI. This PR changes that so console.error will not cause unit
tests to fail anywhere.
@sorenlouv sorenlouv deleted the prevent-console-errors-from-failing-tests branch April 12, 2024 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@achyutjhunjhunwala achyutjhunjhunwala achyutjhunjhunwala approved these changes

@gbamparop gbamparop gbamparop approved these changes

Assignees
No one assigned
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support v8.9.0 v8.10.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@sorenlouv @elasticmachine @apmmachine @kibana-ci @kibanamachine @gbamparop @achyutjhunjhunwala