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

Improve worker's exception handling and related test #4119

Merged
merged 1 commit into from
Aug 12, 2021

Conversation

Martchus
Copy link
Contributor

  • Turn try into eval so the contained return actually returns from
    the enclosing sub
    • No longer log Another error occurred… when that's not actually the
      case
    • No longer stop the worker immediately after the first exception
  • Log error message from the eval block
  • Improve tests
    • Check whether the event loop is actually stopped in a controlled way
    • Check whether all error messages are logged
    • Check whether relevant functions are called exactly as many times as
      expected
  • See https://progress.opensuse.org/issues/96710

* Turn `try` into `eval` so the contained `return` actually returns from
  the enclosing `sub`
    * No longer log `Another error occurred…` when that's not actually the
      case
    * No longer stop the worker immediately after the first exception
* Log error message from the `eval` block
* Improve tests
    * Check whether the event loop is actually stopped in a controlled way
    * Check whether all error messages are logged
    * Check whether relevant functions are called exactly as many times as
      expected
* See https://progress.opensuse.org/issues/96710
@codecov
Copy link

codecov bot commented Aug 11, 2021

Codecov Report

Merging #4119 (895cc5f) into master (de291cd) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4119   +/-   ##
=======================================
  Coverage   97.79%   97.79%           
=======================================
  Files         371      371           
  Lines       33145    33161   +16     
=======================================
+ Hits        32415    32431   +16     
  Misses        730      730           
Impacted Files Coverage Δ
lib/OpenQA/Worker.pm 95.69% <100.00%> (+0.03%) ⬆️
t/24-worker-overall.t 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de291cd...895cc5f. Read the comment docs.

Copy link
Member

@kalikiana kalikiana left a comment

Choose a reason for hiding this comment

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

Good catch on the return. Not sure if counting signal in the test is useful but I guess if this turns out to be flaky that could be useful.

Copy link
Member

@asdil12 asdil12 left a comment

Choose a reason for hiding this comment

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

LGTM

@Martchus Martchus merged commit 3e33436 into os-autoinst:master Aug 12, 2021
@Martchus Martchus deleted the worker-exception-handling branch August 12, 2021 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants