Closed Bug 1291569 Opened 4 years ago Closed 4 years ago

test_system_update doesn't run the tests you think it runs

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: shu, Assigned: rhelmer)

References

Details

Attachments

(1 file)

There's this loop in test_system_update:

for (let setup of Object.keys(TEST_CONDITIONS)) {
  for (let test of Object.keys(TESTS)) {
    add_task(function*() {
      do_print("Running test " + setup + " " + test);

      yield exec_test(TEST_CONDITIONS[setup], TESTS[test]);
    }); 
  }
}

This loop currently runs the pair 'withBothSets' and 'badCert' a bunch of times, because the for-let-of scoping is wrong.

This scoping is fixed by bug 1263355, but that patch breaks this test because as far as I can tell, a bunch of combinations here were never tested and never passed. For starts, 'blank' and 'empty' fails on checking the final state.

This is one of the things currently blocking bug 1263355 from landing, please advise.
See comment 0.
Flags: needinfo?(dtownsend)
Blocks: 1263355
Flags: needinfo?(dtownsend) → needinfo?(rhelmer)
I can reproduce this (moved the for-let-of to be inside the `add_task`) and have narrowed down which tests are failing:

1) overlapping - the signed XPIs used in the test have unexpected version numbers
2) empty - fails in combination with "blank" and "withAppSet"

It'll take some time to get the above fixed, so in the interest of having some test coverage now, and unblocking bug 1263355, I suggest we work around #1 by specifying the right version numbers and #2 by temporarily disabling the "empty" combination of tests.
Assignee: nobody → rhelmer
Status: NEW → ASSIGNED
Flags: needinfo?(rhelmer)
Due to bug 1263355, this test hasn't been running the tests we thought it did.
This patch gets most of the tests running and disables the few that are not
passing while we work on fixing these.

Review commit: https://reviewboard.mozilla.org/r/68948/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68948/
Attachment #8777376 - Flags: review?(aswan)
Comment on attachment 8777376 [details]
Bug 1291569 - avoid for-let-of scoping bug and temporarily disable failing tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68948/diff/1-2/
Comment on attachment 8777376 [details]
Bug 1291569 - avoid for-let-of scoping bug and temporarily disable failing tests

https://reviewboard.mozilla.org/r/68948/#review66030

The fix for the looping error looks good.  r+ with new bug(s) to reinstante the newly uncovered failing tests.
Attachment #8777376 - Flags: review?(aswan) → review+
See Also: → 1291748
See Also: → 1291752
Followups filed, I've taken both of them and will get them fixed up ASAP.

Doing a trying run now, and will land shortly to unblock bug 1263355.
Thanks for the quick action!
Pushed by rhelmer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cf2d25af848d
avoid for-let-of scoping bug and temporarily disable failing tests r=aswan
(In reply to Shu-yu Guo [:shu] from comment #7)
> Thanks for the quick action!

No worries, I am really glad you caught this actually!
https://hg.mozilla.org/mozilla-central/rev/cf2d25af848d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.