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.
Category:
Regressions/Crashes
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.
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.
- 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
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 :/
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.
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.
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
- 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 :)
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...
Also available in: Atom
PDF
