1291569 - test_system_update doesn't run the tests you think it runs

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Shu-yu Guo [:shu]]

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.

Comment 1

[Shu-yu Guo [:shu]]

See comment 0.

Comment 2

[Robert Helmer [: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.

Comment 3

[Robert Helmer [:rhelmer]]

Attachment: Bug 1291569 - avoid for-let-of scoping bug and temporarily disable failing tests

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/

Comment 4

[Robert Helmer [:rhelmer]]

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 5

[Andrew Swan [:aswan]]

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.

Comment 6

[Robert Helmer [:rhelmer]]

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.

Comment 7

[Shu-yu Guo [:shu]]

Thanks for the quick action!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cf2d25af848d
avoid for-let-of scoping bug and temporarily disable failing tests r=aswan

Comment 9

[Robert Helmer [:rhelmer]]

(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!

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cf2d25af848d