action #41510

CSS class mismatch resborder_softfail vs. resborder_softfailed

Added by okurz over 1 year ago. Updated over 1 year ago.

Status:ResolvedStart date:24/09/2018
Priority:NormalDue date:
Assignee:mkittler% Done:

0%

Category:Concrete Bugs
Target version:Done
Difficulty:
Duration:

Description

https://openqa.opensuse.org/tests/759328#step/force_scheduled_tasks/15 shows a softfail popup with the html code

<span class="resborder resborder_softfail">bsc#1063638</span>

however https://github.com/os-autoinst/openQA/blob/master/assets/stylesheets/test-details.scss mentions only softfailed so the CSS classes do not math. Workaround needles set the class correctly to "resborder_softfailed". A second case is https://openqa.opensuse.org/tests/759328#step/grub_set_bootargs/5 which sets the class "resborder_hash(0x9b8b8a0)", certainly also not as expected. The result is that the thumbnail does not show up with a yellow border

History

#1 Updated by mkittler over 1 year ago

The web UI just renders whatever os-autoinst stores as the result (even if it is a hashref which is not so nice of course).

But value expected by the web UI is definitely softfailed. Not sure why only softfail or even a hash is written by os-autoinst.

#2 Updated by mkittler over 1 year ago

Seems like when we get a hashref, it contains a nested result:

   {
      "title" : "Soft Failed",
      "text" : "grub_set_bootargs-7.txt",
      "result" : {
         "frametime" : [
            "82.62",
            "82.67"
         ],
         "screenshot" : "grub_set_bootargs-6.png",
         "result" : "unk"
      },
      "needles" : []
   },

(Excerpt from cat /var/lib/openqa/testresults/02084/02084411-sle-15-SP1-Installer-DVD-s390x-Build47.1-skip_registration@s390x-zVM-ctc/details-grub_set_bootargs.json | json_pp of job https://openqa.suse.de/tests/2084411#step/grub_set_bootargs/5.)

I assume that's a bug in os-autoinst.

#3 Updated by okurz over 1 year ago

Sorry if my bug report confused you because, yes, it is also obvious to me that this is an os-autoinst problem and that openQA makes the right assumptions. Nevertheless, this here is also the ticket reporting system for os-autoinst ;)

#4 Updated by coolo over 1 year ago

Not talking about CSS would have helped :)

#5 Updated by mkittler over 1 year ago

I think the 'even if it is a hashref' point could still be improved on the openQA-side.

I'm also wondering what has changed in os-autoinst. Eg. git log -G'softfail' returns a bunch of commits but git log -G'softfailed' nothing.

#6 Updated by okurz over 1 year ago

Didn't we consolidate the names in openQA some time and actually rename from softfail to softfailed? Maybe it's not a regression but never worked differently

#7 Updated by coolo over 1 year ago

format_result helper is doing this mapping

#8 Updated by mkittler over 1 year ago

format_result is not doing any mapping from the terms currently returned by os-autoinst to openQA terms. It already assumes softfailed like any other place in openQA (or I missed something when grepping). And besides, this helper is only used for the module result but not for the step result (which this issue is about).

#9 Updated by coolo over 1 year ago

  • Target version set to Current Sprint

#10 Updated by mkittler over 1 year ago

  • Status changed from New to In Progress
  • Assignee set to mkittler

I would just handle softfail as test step result in the web UI as well and prevent rendering undef's or references: https://github.com/os-autoinst/openQA/pull/1832

Maybe the web UI previously even accepted softfail and with some CSS refactoring the class got lost.

What's still missing is to find out why os-autoinst sometimes renders a nested detail where the result should be.

#11 Updated by mkittler over 1 year ago

That's what I've found out about the current behavior:

  1. The test API function record_soft_failure always adds the last screenshot info as result where the result should actually just a string like 'ok' or 'fail'. (https://github.com/os-autoinst/os-autoinst/blob/master/basetest.pm#L459)
  2. os-autoinst does not produce any details with the result 'softfail' or 'softfail*ed*'.
  3. If an image is shown as softfailure in the web UI, that is because the web UI itself checks for the workaround flag being present and then sets the result to 'softfail*ed*' on its own. (https://github.com/os-autoinst/openQA/blob/master/lib/OpenQA/WebAPI/Plugin/Helpers.pm#L373)

How I would change it:

  1. record_soft_failure doesn't attempt to add any kind of screenshot info. This test API function is only for recording text. Softfailure screenshots happen via 3.
  2. os-autoinst sets the result of the details produced via record_soft_failure to 'softfail' which is consistent with 'fail' and with the last PR handled by the web UI.
  3. no change

#12 Updated by okurz over 1 year ago

How I would change it:


  1. record_soft_failure doesn't attempt to add any kind of screenshot info. This test API function is only for recording text. Softfailure screenshots happen via 3.
  2. os-autoinst sets the result of the details produced via record_soft_failure to 'softfail' which is consistent with 'fail' and with the last PR handled by the web UI.
  3. no change

Hm, maybe related: What I would like to be able is to override a "failed" status from a job with a call to record_soft_failure. The use case is the following: More and more "softfail" means for me "known issue" not how many other people understand it as "less severe than fail" because we should not or even can not decide about a severity within openQA fails. Reviewing test failures is annoying to many especially when mainly it is about "known issues" which we need to label. So it would be good if we as reviewers can save time and put an explicit record_soft_failure call in the post_fail_hook of jobs so that for known issues the job turns out as "softfail" instead of "fail" with a reference to the issue as shown by record_soft_failure. See #39719

#13 Updated by mkittler over 1 year ago

PR for fixing the os-autoinst side as explained in my comment above: https://github.com/os-autoinst/os-autoinst/pull/1042

@okurz Ok. But I wouldn't change multiple things at a time and track this idea separately.

#14 Updated by mkittler over 1 year ago

  • Status changed from In Progress to Resolved

The PRs have been merged.

Old tests https://openqa.suse.de/tests/2084411#step/grub_set_bootargs/5 are only rendered with grey color so far but at least the class isn't HASH(...) anymore. So the fix only applies to new tests (as soon as os-autoinst is deployed).

#15 Updated by coolo over 1 year ago

  • Target version changed from Current Sprint to Done

Also available in: Atom PDF