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

Simplify handling of potential not-implemented states of jobs #3877

Merged
merged 1 commit into from May 10, 2021

Conversation

okurz
Copy link
Member

@okurz okurz commented Apr 30, 2021

Helps with statement coverage in tests as well :)

Encountered while looking at code statement coverage reports on codecov,
in particular
https://codecov.io/gh/os-autoinst/openQA/src/master/lib/OpenQA/BuildResults.pm

Related progress issue: https://progress.opensuse.org/issues/55364

Copy link
Contributor

@Martchus Martchus left a comment

Choose a reason for hiding this comment

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

In production this will result in a 500 error page for the user and we'd still have to dig into the logs to find the exact error message. That's also not very nice. I'd rather put a warning somewhere on the web page if we consider this important enough or completely remove the code path. Or we'd simply add a category "other" where we would add the states/results to.

@okurz
Copy link
Member Author

okurz commented Apr 30, 2021

Right, so if the die is handled that way by presenting 500 to the user then surely this PR in the current form is no improvement. I have a different preference though. An error in the logs is still better than in the webUI. The webUI is meant for users, the log for admins. That we don't look into logs (anymore) is our problem as admins. For that we also have https://github.com/os-autoinst/openqa-logwarn . But maybe an adjustment of your "other" proposal we can just count the unidentified as "unfinished" together. See my updated proposal.

Copy link
Contributor

@Martchus Martchus left a comment

Choose a reason for hiding this comment

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

I guess counting these into unfinished makes sense. We handle already cancelled and done and it is unlikely that we introduce more "finished" states in the future.

@Martchus Martchus changed the title Make potential not-implemented states of jobs obvious with die Better handle potential not-implemented states of jobs Apr 30, 2021
@kalikiana
Copy link
Member

Can you please add a description with e.g. a ticket or discription of what you're trying to achieve?

@okurz
Copy link
Member Author

okurz commented May 5, 2021

Is the second line "Helps with statement coverage in tests as well :)" in the commit message enough?

@kalikiana
Copy link
Member

Is the second line "Helps with statement coverage in tests as well :)" in the commit message enough?

Not really. I'm still wondering what conversation or investigation this helps with "as well".

Helps with statement coverage in tests as well :)

Encountered while looking at code statement coverage reports on codecov,
in particular
https://codecov.io/gh/os-autoinst/openQA/src/master/lib/OpenQA/BuildResults.pm

Related progress issue: https://progress.opensuse.org/issues/55364
@okurz okurz changed the title Better handle potential not-implemented states of jobs Simplify handling of potential not-implemented states of jobs May 5, 2021
@okurz
Copy link
Member Author

okurz commented May 5, 2021

@kalikiana updated the git commit message with more details and also a ticket reference and copied that into the PR description.

@okurz
Copy link
Member Author

okurz commented May 5, 2021

created https://progress.opensuse.org/issues/92164 for the test failure which I consider unrelated. Will retrigger.

@kalikiana
Copy link
Member

kalikiana commented May 10, 2021

I guess it's still failing because of poo#92164 although #3890 got merged - re-triggered again, let's see:

11:17:17] t/01-test-utilities.t ..................................... 
#   Failed test 'test would have failed'
#   at t/01-test-utilities.t line 49.

#   Failed test 'manual termination via stop_service does not trigger _fail_and_exit'
#   at t/01-test-utilities.t line 63.
#          got: '1'
#     expected: '0'
# Looks like you failed 2 tests of 5.

@codecov
Copy link

codecov bot commented May 10, 2021

Codecov Report

Merging #3877 (fe29e53) into master (2ab3ac5) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3877   +/-   ##
=======================================
  Coverage   96.75%   96.76%           
=======================================
  Files         368      368           
  Lines       32798    32796    -2     
=======================================
  Hits        31735    31735           
+ Misses       1063     1061    -2     
Impacted Files Coverage Δ
lib/OpenQA/BuildResults.pm 98.40% <100.00%> (+1.54%) ⬆️

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 2ab3ac5...fe29e53. Read the comment docs.

@mergify mergify bot merged commit 79d2026 into os-autoinst:master May 10, 2021
@okurz okurz deleted the feature/missing_state branch May 10, 2021 14:45
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