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

Fix carry-over only due to one matching bugref in step title #3988

Merged
merged 4 commits into from Jun 30, 2021

Conversation

Martchus
Copy link
Contributor

@Martchus Martchus commented Jun 29, 2021

  • Fix carry-over only due to one matching bugref in step title
    • Prevent carry-over despite a mismatch of failing test modules like in
      https://progress.opensuse.org/issues/94880
    • Still allow carry-over when the same bug is found via different modules
      (ef5f07c) but only if the other failing
      test modules match as well
  • Consider all modules on carry over, despite missing/broken results
    • If it isn't possible to read the results/details of some (soft)failed
      modules that doesn't mean we should ignore them completely. In addition,
      further (soft)failed modules should not be ignored as well.

I've been adding tests now. I've also been testing that tests fail without the change (as there was already a similar test in place which wasn't good enough).

@okurz
Copy link
Member

okurz commented Jun 29, 2021

I like the code style change :)

@Martchus Martchus marked this pull request as ready for review June 30, 2021 10:06
@codecov
Copy link

codecov bot commented Jun 30, 2021

Codecov Report

Merging #3988 (a8b56d1) into master (65cd968) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3988      +/-   ##
==========================================
- Coverage   96.93%   96.90%   -0.04%     
==========================================
  Files         370      370              
  Lines       32968    32971       +3     
==========================================
- Hits        31959    31949      -10     
- Misses       1009     1022      +13     
Impacted Files Coverage Δ
lib/OpenQA/Schema/Result/Jobs.pm 98.50% <100.00%> (-0.01%) ⬇️
t/17-labels_carry_over.t 100.00% <100.00%> (ø)
t/lib/OpenQA/Test/Utils.pm 78.37% <0.00%> (-3.66%) ⬇️

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 65cd968...a8b56d1. Read the comment docs.

Copy link
Member

@Vogtinator Vogtinator left a comment

Choose a reason for hiding this comment

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

The algorithm seems sound to me.

The only weakness I could identify is that when a failed module gains a bugref in test code, that blocks carryover. Shouldn't really be an issue in practice, and the previous code had that as well.

@Martchus
Copy link
Contributor Author

The only weakness I could identify is that when a failed module gains a bugref in test code, that blocks carryover. Shouldn't really be an issue in practice, and the previous code had that as well.

I thought about it as well but thought it wasn't worth it as it would complicate the code a bit. We can still implement it later if needed.

t/17-labels_carry_over.t Outdated Show resolved Hide resolved
Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

Sorry, I don't understand. What has the "carry-over" have to do with "matching bugref in step title"? I thought carry-over is only looking at failed modules, not steps?

@Martchus
Copy link
Contributor Author

I thought carry-over is only looking at failed modules, not steps?

That is wrong. With ef5f07c (referenced in the commit message) it has been changed to also take bugrefs within step titles into account.

* Prevent carry-over despite a mismatch of failing test modules like in
  https://progress.opensuse.org/issues/94880
* Still allow carry-over when the same bug is found via different modules
  (ef5f07c) but only if the other failing
  test modules match as well
* If it isn't possible to read the results/details of some (soft)failed
  modules that doesn't mean we should ignore them completely. In addition,
  further (soft)failed modules should not be ignored as well.
* See https://progress.opensuse.org/issues/94880
@mergify mergify bot merged commit 597771a into os-autoinst:master Jun 30, 2021
@Martchus Martchus deleted the fix-carry-over branch June 30, 2021 15:10
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

4 participants