Closed
Bug 1203452
Opened 9 years ago
Closed 9 years ago
Use more descriptive assertion messages for update tests (e.g. "Update has been found" vs. "AssertionError")
Categories
(Testing :: Firefox UI Tests, defect)
Tracking
(firefox41 fixed, firefox42 fixed, firefox43 fixed, firefox44 fixed, firefox-esr38 fixed)
RESOLVED
FIXED
mozilla44
People
(Reporter: whimboo, Assigned: whimboo)
References
Details
Attachments
(1 file)
The failure details are not satisfying given that we do not report any details about that failure. Reason is that we do a simple assert:
> assert self.deck.selected_panel == self.deck.check_for_updates
AssertionError
We should better use the unittest type assertions if possible instead.
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 4•9 years ago
|
||
So far the failure only happened with Firefox 42 and other locales than en-US and Windows 8 x64. Something I would propose is to fix the assertion output for all branches and then try to figure out what's causing this problem if it happens more than that.
status-firefox42:
--- → affected
OS: Unspecified → Windows 8
Hardware: Unspecified → x86_64
Summary: Intermittent failure in test_direct_update.py / test_fallback_update.py: AssertionError → Intermittent failure in test_direct_update.py / test_fallback_update.py: AssertionError (self.deck.selected_panel == self.deck.check_for_updates)
Assignee | ||
Comment 5•9 years ago
|
||
Looking more into one of the build results shows me that the direct update test which runs first does not even check for updates but directly starts to download it:
00:01:10.156 *** AUS:SVC readStatusFile - status: downloading, path: C:\Users\mozauto\AppData\Local\Mozilla\Firefox\firefox\updates\0\update.status
As you can see in the path the build is getting installed into a shared location. So most likely a former build did not uninstall it and the update.status file survived. That means what we have to do for now is to make use of the workspace option, which i have added recently on bug 1199574. With that we can install Firefox into the Jenkins workspace which definetely gets cleaned up before each build.
To fix the workspace issue I filed: https://github.com/mozilla/mozmill-ci/issues/643.
Otherwise I cleaned-up the win-81-62-2 box from the remaining updates folder, so updates should work again now.
Assignee | ||
Comment 6•9 years ago
|
||
So the remaining issue are the better assertion descriptions, which are necessary to better see what's wrong with failing update tests. I will morph the summary and work on a fix to get it improved across all branches.
Armen, I believe this is also wanted by RelEng. They most likely don't want to check the logs all the time but have a crisp failure message. Maybe that fix might block the production use?
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
status-firefox40:
--- → affected
status-firefox41:
--- → affected
status-firefox43:
--- → affected
status-firefox-esr38:
--- → affected
OS: Windows 8 → All
Hardware: x86_64 → All
Summary: Intermittent failure in test_direct_update.py / test_fallback_update.py: AssertionError (self.deck.selected_panel == self.deck.check_for_updates) → Use more descriptive assertion messages for update tests (e.g. "Update has been found" vs. "AssertionError")
Comment 7•9 years ago
|
||
That would be nice.
I give them a summary of all jobs in here:
https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/scripts/firefox_ui_tests/update_release.py?offset=500#318
Perhaps a file with an extended summary or even an html summary could be generated and uploaded as an artifact.
This is a scope creep.
Assignee | ||
Comment 8•9 years ago
|
||
Armen that's not what this bug is actually about. It's more about what actual assertions in the test use as description, so it's immediately visible what's broken.
So one example would be to not only see "AssertionError" as failure but "downloading != checkforupdates".
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 107•9 years ago
|
||
The underlying issue for this massive fallout was bug 1206617 which also let the update check stuck.
I will work on this bug so we can see better failure details on treeherder.
Assignee | ||
Comment 108•9 years ago
|
||
After talking to David on IRC the best would be to move all plain assert calls out of our libraries and if possible create helper methods on the appropriate test class. This would work fine for the update tests, which I mainly cover via this bug report.
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 124•9 years ago
|
||
This patch fixes the those assertions which didn't supply any details of what's being checked yet. Some messages I had to extend to also show the expected vs. actual values. That was necessary at least until we have better assertion message displaying for our tests. I filed bug 1207067 for some improvements.
Attachment #8664125 -
Flags: review?(dburns)
Updated•9 years ago
|
Attachment #8664125 -
Flags: review?(dburns) → review+
Assignee | ||
Comment 125•9 years ago
|
||
PR merged to master:
https://github.com/mozilla/firefox-ui-tests/commit/66c5753a057610df0bf1d5b8a9080ccf4e93e31c
I will watch the results for at least today before cherry-picking the patch for the other branches.
Assignee | ||
Comment 126•9 years ago
|
||
I cherry-picked the commit for all the other branches:
https://github.com/mozilla/firefox-ui-tests/commit/f496fb2bc1cd327df8ae7454c76a16e600599caa (aurora)
https://github.com/mozilla/firefox-ui-tests/commit/91bc194a93f86361051057fcc9c58b8ec768d836 (beta)
https://github.com/mozilla/firefox-ui-tests/commit/da1a43b6f974b683822ae1e0e9c4ebe53e29de3d (release)
https://github.com/mozilla/firefox-ui-tests/commit/b27e1c9e9fbb19c7f8f20bb31802c4c6ab859ff9 (esr38)
This is actually a very good improvement for bug 1182796. So adding it to the dependency tree.
Updated•9 years ago
|
Product: Mozilla QA → Testing
You need to log in
before you can comment on or make changes to this bug.
Description
•