Project

General

Profile

Actions

action #58959

closed

needle diff doesn't work right in current git for needles in subdirectories

Added by AdamWill over 4 years ago. Updated over 4 years ago.

Status:
Resolved
Priority:
High
Assignee:
Category:
Regressions/Crashes
Target version:
-
Start date:
2019-11-01
Due date:
% Done:

0%

Estimated time:

Description

So I just updated Fedora's openQA and os-autoinst packages to current git, and deployed it on our staging instance for testing. Things are mostly working so far, except needle diff - the thing where you can see in the web UI why a needle doesn't match by scrubbing a bar over a view that compares the needle and the screen it's being compared to - just doesn't work right at all. You can see this on any needle match at https://openqa.stg.fedoraproject.org/ . It seems like the bar you can drag is there, but invisible - if you move the cursor to the middle of the needle it'll change to a cursor indicating you can drag, and you can drag the bar somewhere else and now the cursor will change when it's at that position. But you can't see the bar, and you can't see any of the match area comparisons you would usually see.

I'm a bit baffled trying to debug this, as there are no errors showing on the browser console and assets/javascripts/needlediff.js has not changed since January (the last time I updated the Fedora snapshots was early August, and those builds were fine). But it's definitely broken! It's broken on Firefox and Epiphany, for me. I don't have Chrome installed.

Actions #1

Updated by AdamWill over 4 years ago

It seems we're hitting this early bailout in the 'draw' function in needlediff.js:

// Then, check if there is a needle to compare with
if (!this.needleImg) {
return;
}

but I'm not sure why.

Actions #2

Updated by AdamWill over 4 years ago

Seems like it's a needle URL loading issue. Check this:

WORKS (prod, older code): https://openqa.fedoraproject.org/needles/fedora/install_lang-cantarell111.png?version=Rawhide&jsonfile=%2Fvar%2Flib%2Fopenqa%2Fshare%2Ftests%2Ffedora%2Fneedles%2Fanaconda%2Flang_select%2Finstall_lang-cantarell111.json
FAILS (stg, newer code): https://openqa.fedoraproject.org/needles/fedora/install_lang-cantarell111.png?version=Rawhide&jsonfile=%2Fvar%2Flib%2Fopenqa%2Fshare%2Ftests%2Ffedora%2Fneedles%2Fanaconda%2Flang_select%2Finstall_lang-cantarell111.json

exact same URL except for the host name, but works on the older code, fails on master. That's the URL we're ultimately using to try and load the needle image from, in the needlediff code; it breaks because that doesn't work.

Actions #3

Updated by AdamWill over 4 years ago

So that means it's looking like this commit is the culprit:

https://github.com/os-autoinst/openQA/commit/36aa9744392a61bdd794c2de75c1ad24143920a6

I'm gonna bet that doesn't handle needles in subdirectories, or something.

Actions #4

Updated by AdamWill over 4 years ago

  • Subject changed from needle diff doesn't work right in current git to needle diff doesn't work right in current git for needles in subdirectories
Actions #5

Updated by AdamWill over 4 years ago

So basically it seems martchus wanted to drop the old approach of getting the path to the needle via needle_info() as part of being "more flexible regarding custom test runs", but just assuming the needle will always be present at needledir($distri, $version) . $name . $format is too naive. We need a better way, but I'm not yet sure what the best fix would be :/

Actions #6

Updated by AdamWill over 4 years ago

https://github.com/os-autoinst/openQA/pull/2456 is my best effort at fixing this, but someone else might be able to think of something better.

Actions #7

Updated by okurz over 4 years ago

thanks for the PR to fix it. It shouldn't be merged currently as tests fail. If you manage to fix the tests we are good to merge it. However we should keep this ticket still open afterwards until we have tests to cover the feature of needle subdirectories. As an alternative we could try to revert the relevant changes from https://github.com/os-autoinst/openQA/commit/36aa9744392a61bdd794c2de75c1ad24143920a6 for the time being.

Actions #8

Updated by AdamWill over 4 years ago

so I updated the PR, but honestly it made me wonder if we should just quit allowing needles in subdirectories, there are other things there that clearly don't work, and since SUSE doesn't put needles in subdirectories I suspect future changes will forget about that use case too.

Actions #9

Updated by coolo over 4 years ago

I find subdirectories rather useful - and would actually prefer if we could have SUSE put needles in directories as well. Most tools really suffer with 5000+ files in one directory

Actions #10

Updated by okurz over 4 years ago

  • Status changed from New to Resolved
  • Assignee set to AdamWill

so you managed to rebase – which should fix the full stack test for now – and also added new tests covering your changed code, all good :)

Actions #11

Updated by AdamWill over 4 years ago

if we're gonna keep it it'd probably be good for someone to look through all the needle-handling-stuff from square 1 and fix any weirdnesses related to subdirectory needles, then, like the ones I commented...

Actions

Also available in: Atom PDF