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

[QA][Code Coverage] Drop catchError and use try / catch instead, #69198

Merged
merged 9 commits into from
Jun 30, 2020

Conversation

wayneseymour
Copy link
Member

@wayneseymour wayneseymour commented Jun 15, 2020

to fail the build on test run and / or ingestion.

Summary

We no longer wish to be notified for unstable builds. However, if the coverage
ingestion fails, we want the build to fail and slack notify our team.

to fail the build on test run and / or ingestion.
@wayneseymour wayneseymour added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes test-coverage issues & PRs for improving code test coverage labels Jun 15, 2020
@wayneseymour wayneseymour self-assigned this Jun 15, 2020
Copy link
Member Author

@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.

@dmlemeshko Perhaps we should only wrap the handleIngestion fn with the try / catch? Your thoughts man?

@wayneseymour wayneseymour linked an issue Jun 15, 2020 that may be closed by this pull request
@dmlemeshko
Copy link
Member

dmlemeshko commented Jun 15, 2020

@dmlemeshko Perhaps we should only wrap the handleIngestion fn with the try / catch? Your thoughts man?

I think it won't hurt to keep runTest() as well:

  • If jest fails, we are not getting coverage reports and no need to ingest, return red build
  • If functional fails, we proceed unless moving files fails and no chance for ingestion again, return the red build
// Worker for running functional tests. Runs a setup process (e.g. the kibana build) then executes a map of closures in parallel (e.g. one for each ciGroup)
def functional(name, Closure setup, Map processes) {
  return {
    parallelProcesses(name: name, setup: setup, processes: processes, delayBetweenProcesses: 20, size: 'xl')
  }
}

I don't see failure is handled here, so probably we must wrap it up to handle and report failure

@dmlemeshko
Copy link
Member

dmlemeshko commented Jun 15, 2020

@wayneseymour could you push a breaking commit to see it really works for broken ingestion/incorrect vault data?

@wayneseymour
Copy link
Member Author

@wayneseymour could you push a breaking commit to see it really works for broken ingestion/incorrect vault data?

Yeah great idea!

@wayneseymour
Copy link
Member Author

Last build failed, now testing: https://kibana-ci.elastic.co/job/elastic+kibana+qa-research/11/console

@wayneseymour
Copy link
Member Author

@wayneseymour wayneseymour marked this pull request as ready for review June 16, 2020 21:00
@wayneseymour
Copy link
Member Author

wayneseymour commented Jun 16, 2020

@dmlemeshko @LeeDr This job keeps breaking with:

15:02:33  repoInfo: [branch:fail-build-on-coverage-ingestion-error, targetBranch:null, commit:a679685bcda1e7bca9ee562b60c82857b07139d9]

I'm not sure what's going on.

From: https://kibana-ci.elastic.co/job/elastic+kibana+qa-research/12/console

@wayneseymour wayneseymour changed the title Drop catchError and use try / catch instead, [QA][Code Coverage] Drop catchError and use try / catch instead, Jun 17, 2020
@wayneseymour
Copy link
Member Author

@dmlemeshko @LeeDr This job keeps breaking with:

15:02:33  repoInfo: [branch:fail-build-on-coverage-ingestion-error, targetBranch:null, commit:a679685bcda1e7bca9ee562b60c82857b07139d9]

I'm not sure what's going on.

From: kibana-ci.elastic.co/job/elastic+kibana+qa-research/12/console

@dmlemeshko ideas?

@wayneseymour
Copy link
Member Author

@elasticmachine merge upstream

@wayneseymour
Copy link
Member Author

@wayneseymour
Copy link
Member Author

Testing: kibana-ci.elastic.co/job/elastic+kibana+qa-research/23

Copy link
Member

@dmlemeshko dmlemeshko left a comment

Choose a reason for hiding this comment

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

Code LGTM. Interesting to hear Brian opinion :)

@wayneseymour
Copy link
Member Author

@dmlemeshko Yeah I think @brianseeders can affirm whether our assumptions of catchError is correct

@brianseeders
Copy link
Contributor

catchError { doSomething() }

is basically the same as

try {
  doSomething()
} catch(err) {
  // Set the build status to failure
  // Print the error
  // Maybe other stuff?
}

So, I don't think you need this PR at all, unless I'm not understanding your intentions.

@wayneseymour
Copy link
Member Author

@brianseeders ok, well one consideration is that we do not want to be notified for unstable builds, just failed builds.

@brianseeders
Copy link
Contributor

@brianseeders ok, well one consideration is that we do not want to be notified for unstable builds, just failed builds.

You could add another condition to handleFail() that checks for UNSTABLE

update handleFail fn to not slack notifiy
if the build is unstable, per Brian S.
@wayneseymour
Copy link
Member Author

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

@wayneseymour wayneseymour requested a review from LeeDr June 30, 2020 23:07
@wayneseymour wayneseymour merged commit f428b2d into master Jun 30, 2020
@wayneseymour wayneseymour deleted the fail-build-on-coverage-ingestion-error branch June 30, 2020 23:07
Bamieh pushed a commit to Bamieh/kibana that referenced this pull request Jul 1, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 1, 2020
…-based-rbac

* upstream/master: (38 commits)
  Move logger configuration integration test to jest (elastic#70378)
  Changes observability plugin codeowner (elastic#70439)
  update (elastic#70424)
  [Logs UI] Avoid CCS-incompatible index name resolution (elastic#70179)
  Enable "Explore underlying data" actions for Lens visualizations (elastic#70047)
  Initial work on uptime homepage API (elastic#70135)
  expressions indexPattern function (elastic#70315)
  [Discover] Deangularization context error message refactoring (elastic#70090)
  [Lens] Add "no data" popover (elastic#69147)
  [Lens] Move chart switcher over (elastic#70182)
  chore: add missing mjs extension (elastic#70326)
  [Lens] Multiple y axes (elastic#69911)
  skip flaky suite (elastic#70386)
  fix bug to add timeline to case (elastic#70343)
  [QA][Code Coverage] Drop catchError and use try / catch instead, (elastic#69198)
  [QA] [Code Coverage] Integrate with Team Assignment Pipeline and Add Research and Development Indexes and Cluster (elastic#69348)
  [Metrics UI] Add context.reason and alertOnNoData to Inventory alerts (elastic#70260)
  Resolver refactoring (elastic#70312)
  [Ingest Manager] Fix agent ack after input format change (elastic#70335)
  [eslint][ts] Enable prefer-ts-expect-error (elastic#70022)
  ...
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 69198 or prevent reminders by adding the backport:skip label.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jul 2, 2020
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 69198 or prevent reminders by adding the backport:skip label.

1 similar comment
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 69198 or prevent reminders by adding the backport:skip label.

@LeeDr LeeDr added the backport:skip This commit does not require backporting label Jul 7, 2020
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes test-coverage issues & PRs for improving code test coverage v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[QA] [Code Coverage] Fail build on code coverage ingestion error
6 participants