action #58959
closedneedle diff doesn't work right in current git for needles in subdirectories
0%
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.
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.
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.
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.
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
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 :/
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.
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.
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.
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
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 :)
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...