Automated Test Results view doesn't catch incorrect waivers #5397
Comments
|
Just saw a similar issue with https://bodhi.fedoraproject.org/updates/FEDORA-2023-de5276064a where the Automated Tests tab was showing all the failures as waived, even though several of them actually had not been waived yet. Not sure why that is yet, will look at it. |
|
Haaaar. I think I see why it happens now. In At no point do we take the subject into account at all when constructing the waiver list. So if a given test case has been waived for any subject in the update, when we're in So turns out I was right to comment on this bug, because ultimately it's the same issue: we're not taking subjects into account properly when parsing waivers in the frontend. I think the way to fix this is more or less to construct the waiver hash by subject then testcase, not just by testcase, and rejig |
|
Actually, there should be a much more reliable way to do this. Instead of primarily parsing the waivers - which means we have to reconstruct the work greenwave does to decide which waivers actually apply to which subjects - we can just parse the satisfied requirements. Maybe this wasn't the case when this code was written, but these days, the entry in greenwave's So instead of parsing the waivers we can parse the satisfied requirements, and when we see a waived one, just use the waiver ID to get the text description from the waivers... edit: thinking about it, that would also partly address #5414 , which is nice. |
|
I wonder if it would be better to migrate this complex javascript logic into a python based endpoint and use javascript only to load the output of the endpoint... this way we could write down tests. I also think the code could be easier to maintain. |
|
For a while I kinda wanted to burn down all this "have the frontend query greenwave itself and do lots of stuff with the results" code and just have the frontend get all the necessary info from the backend, but that turned out to be rather difficult because the backend would wind up kinda becoming a kinda shadow combined resultsdb and waiverdb, with a huge store of results and waivers that would need updating constantly...currently the backend code that talks to greenwave doesn't get or parse the test results, it does a non-verbose query and only considers the gating status... Are you suggesting we should have an API endpoint that the frontend can hit to get the backend to do a greenwave query, parse the results, and send nicely-parsed info to the frontend to render? or is it more that the frontend talks to greenwave then sends the data it gets to the backend for the backend to parse? or...do we make it so any time I guess I'm just having trouble visualizing how exactly the flows would work. |
|
I have to first understand exactly what the js code does (I never looked too deep into it). |
|
So basically we'd do things more or less the way we currently do - render most of the page immediately, then do the greenwave query live and populate the Automated Results tab (and improve the gating status text) as the results come in - but move the actual work of doing the query and processing the results into the backend, via an API endpoint that the frontend code would hit while rendering the page? I guess that adds an extra frontend<->backend round trip over what we have right now, right? But it might ultimately wind up being faster because all the greenwave queries would happen in the backend, which is very network-close to greenwave, rather than happening between the user's system and greenwave, which is a much longer roundtrip...so long as the backend could cope with the additional load of doing all that greenwave querying and parsing... |
|
I guess it might also let us keep a closer sync between the frontend and backend's idea of the gating status, because as a byproduct we could have the update's gating status in the backend updated each time anyone hits the page for the update...since we're hitting greenwave anyway we should be able to do that for free. |
|
as to what the JS code does: it queries greenwave (using information passed in from the backend to construct the actual query - it calls It also uses it to construct the entire contents of the Automated Tests tab, which is the most complex thing it does. It needs to show a row for every test result it gets, but also a row for any 'missing' results, which it finds from the 'unsatisfied requirements' from greenwave. It also shows little emblems indicating whether each test result is "required" in a gating policy (ones that are 'required' get an asterisk) and whether failed results have been waived, which is the bit we're concerned with here (waived failures get a thumbs-up). |
…-infra#5397) On the Automated Tests tab, the web UI attempts to indicate when a failed test has been waived. However, it can get this wrong in several ways. It stores the waivers only by testcase - not by subject or scenario - so if greenwave returns any waiver for the testcase, it will mark any failure of that testcase for the update as 'waived'. This can be incorrect if: * The waiver was filed against the wrong subject and is invalid * The waiver is for a different subject (different Koji build) * The waiver is for a different scenario This should fix all those problems. Instead of just parsing the waivers, we primarily parse the satisfied requirements, and when one is of type "test-result-failed-waived" - meaning it's a failure that was waived - we store it in a nested hash of waived failures. The outer hash's keys are subject identifiers, and its values are hashes. In these inner hashes, the keys are the testcase and scenario combined, and the values are the IDs of waivers for that subject/testcase/scenario combination. We keep a separate hash of waivers, the keys being the waiver IDs and the values being the waivers. So when we are constructing the result rows, we can check whether there is a waived failure for the result's testcase, scenario and subject, and only if so, we add the 'waived' marker, constructing the necessary info from the waiver retrieved from the waivers hash. Signed-off-by: Adam Williamson <[email protected]>
|
While we think about that, I went ahead and wrote a PR which I hope should fix this (though as you note, writing this much javascript without a test suite for a safety net is no fun). |
On the Automated Tests tab, the web UI attempts to indicate when a failed test has been waived. However, it can get this wrong in several ways. It stores the waivers only by testcase - not by subject or scenario - so if greenwave returns any waiver for the testcase, it will mark any failure of that testcase for the update as 'waived'. This can be incorrect if: * The waiver was filed against the wrong subject and is invalid * The waiver is for a different subject (different Koji build) * The waiver is for a different scenario This should fix all those problems. Instead of just parsing the waivers, we primarily parse the satisfied requirements, and when one is of type "test-result-failed-waived" - meaning it's a failure that was waived - we store it in a nested hash of waived failures. The outer hash's keys are subject identifiers, and its values are hashes. In these inner hashes, the keys are the testcase and scenario combined, and the values are the IDs of waivers for that subject/testcase/scenario combination. We keep a separate hash of waivers, the keys being the waiver IDs and the values being the waivers. So when we are constructing the result rows, we can check whether there is a waived failure for the result's testcase, scenario and subject, and only if so, we add the 'waived' marker, constructing the necessary info from the waiver retrieved from the waivers hash. Signed-off-by: Adam Williamson <[email protected]>
…-infra#5397) On the Automated Tests tab, the web UI attempts to indicate when a failed test has been waived. However, it can get this wrong in several ways. It stores the waivers only by testcase - not by subject or scenario - so if greenwave returns any waiver for the testcase, it will mark any failure of that testcase for the update as 'waived'. This can be incorrect if: * The waiver was filed against the wrong subject and is invalid * The waiver is for a different subject (different Koji build) * The waiver is for a different scenario This should fix all those problems. Instead of just parsing the waivers, we primarily parse the satisfied requirements, and when one is of type "test-result-failed-waived" - meaning it's a failure that was waived - we store it in a nested hash of waived failures. The outer hash's keys are subject identifiers, and its values are hashes. In these inner hashes, the keys are the testcase and scenario combined, and the values are the IDs of waivers for that subject/testcase/scenario combination. We keep a separate hash of waivers, the keys being the waiver IDs and the values being the waivers. So when we are constructing the result rows, we can check whether there is a waived failure for the result's testcase, scenario and subject, and only if so, we add the 'waived' marker, constructing the necessary info from the waiver retrieved from the waivers hash. Signed-off-by: Adam Williamson <[email protected]>
…-infra#5397) On the Automated Tests tab, the web UI attempts to indicate when a failed test has been waived. However, it can get this wrong in several ways. It stores the waivers only by testcase - not by subject or scenario - so if greenwave returns any waiver for the testcase, it will mark any failure of that testcase for the update as 'waived'. This can be incorrect if: * The waiver was filed against the wrong subject and is invalid * The waiver is for a different subject (different Koji build) * The waiver is for a different scenario This should fix all those problems. Instead of just parsing the waivers, we primarily parse the satisfied requirements, and when one is of type "test-result-failed-waived" - meaning it's a failure that was waived - we store it in a nested hash of waived failures. The outer hash's keys are subject identifiers, and its values are hashes. In these inner hashes, the keys are the testcase and scenario combined, and the values are the IDs of waivers for that subject/testcase/scenario combination. We keep a separate hash of waivers, the keys being the waiver IDs and the values being the waivers. So when we are constructing the result rows, we can check whether there is a waived failure for the result's testcase, scenario and subject, and only if so, we add the 'waived' marker, constructing the necessary info from the waiver retrieved from the waivers hash. Signed-off-by: Adam Williamson <[email protected]>
On the Automated Tests tab, the web UI attempts to indicate when a failed test has been waived. However, it can get this wrong in several ways. It stores the waivers only by testcase - not by subject or scenario - so if greenwave returns any waiver for the testcase, it will mark any failure of that testcase for the update as 'waived'. This can be incorrect if: * The waiver was filed against the wrong subject and is invalid * The waiver is for a different subject (different Koji build) * The waiver is for a different scenario This should fix all those problems. Instead of just parsing the waivers, we primarily parse the satisfied requirements, and when one is of type "test-result-failed-waived" - meaning it's a failure that was waived - we store it in a nested hash of waived failures. The outer hash's keys are subject identifiers, and its values are hashes. In these inner hashes, the keys are the testcase and scenario combined, and the values are the IDs of waivers for that subject/testcase/scenario combination. We keep a separate hash of waivers, the keys being the waiver IDs and the values being the waivers. So when we are constructing the result rows, we can check whether there is a waived failure for the result's testcase, scenario and subject, and only if so, we add the 'waived' marker, constructing the necessary info from the waiver retrieved from the waivers hash. Signed-off-by: Adam Williamson <[email protected]>
On the Automated Tests tab, the web UI attempts to indicate when a failed test has been waived. However, it can get this wrong in several ways. It stores the waivers only by testcase - not by subject or scenario - so if greenwave returns any waiver for the testcase, it will mark any failure of that testcase for the update as 'waived'. This can be incorrect if: * The waiver was filed against the wrong subject and is invalid * The waiver is for a different subject (different Koji build) * The waiver is for a different scenario This should fix all those problems. Instead of just parsing the waivers, we primarily parse the satisfied requirements, and when one is of type "test-result-failed-waived" - meaning it's a failure that was waived - we store it in a nested hash of waived failures. The outer hash's keys are subject identifiers, and its values are hashes. In these inner hashes, the keys are the testcase and scenario combined, and the values are the IDs of waivers for that subject/testcase/scenario combination. We keep a separate hash of waivers, the keys being the waiver IDs and the values being the waivers. So when we are constructing the result rows, we can check whether there is a waived failure for the result's testcase, scenario and subject, and only if so, we add the 'waived' marker, constructing the necessary info from the waiver retrieved from the waivers hash. Signed-off-by: Adam Williamson <[email protected]> (cherry picked from commit 8ae22c4)
On the Automated Tests tab, the web UI attempts to indicate when a failed test has been waived. However, it can get this wrong in several ways. It stores the waivers only by testcase - not by subject or scenario - so if greenwave returns any waiver for the testcase, it will mark any failure of that testcase for the update as 'waived'. This can be incorrect if: * The waiver was filed against the wrong subject and is invalid * The waiver is for a different subject (different Koji build) * The waiver is for a different scenario This should fix all those problems. Instead of just parsing the waivers, we primarily parse the satisfied requirements, and when one is of type "test-result-failed-waived" - meaning it's a failure that was waived - we store it in a nested hash of waived failures. The outer hash's keys are subject identifiers, and its values are hashes. In these inner hashes, the keys are the testcase and scenario combined, and the values are the IDs of waivers for that subject/testcase/scenario combination. We keep a separate hash of waivers, the keys being the waiver IDs and the values being the waivers. So when we are constructing the result rows, we can check whether there is a waived failure for the result's testcase, scenario and subject, and only if so, we add the 'waived' marker, constructing the necessary info from the waiver retrieved from the waivers hash. Signed-off-by: Adam Williamson <[email protected]> (cherry picked from commit 8ae22c4)
Someone manually filed incorrect waivers for https://bodhi.fedoraproject.org/updates/FEDORA-2023-9a0b560d10 . They filed waivers for Fedora CI tests (whose subject is the package NVR), but filed the waivers with the subject as the update alias.
This meant that greenwave didn't consider the waivers valid and the update was still gated. However, the Automated Test Results view showed a 'thumbs-up' in the row for each failed test, with a mouseover indicating the failure had been waived.
This is because the code in
make_rowfor adding this "update has been waived" icon is unsophisticated. It just checks whether there is any waiver for the testcase, and considers the failure waived if there is. It doesn't check whether the waiver is for the correct subject.I'm filing an issue not a PR because I looked at writing a PR and it's kind of a pain (it's not just an easy one-liner, you'd have to rejig things a bit), so I'm prioritizing other stuff right now. But I wanted to flag the issue in case anyone else wants to fix it.
@vkadlcik for reference (if we make this better, Bodhi wouldn't seem to suggest you'd successfully waived the failures when you hadn't, and might even be able to tell you you filed a bad waiver...)
The text was updated successfully, but these errors were encountered: