TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\migration-to-rdf-ui-2\test-migrate-to-rdf-ui-2.js | test-migrate-to-rdf-ui-2.js::test_width_persisted First seen Sat Jun 10, 2017, 6:36:42: https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=3e3745b52dc53eb74efd73d3107a81e2e13f94be Log says: https://archive.mozilla.org/pub/thunderbird/tinderbox-builds/comm-central-win32/1497069442/comm-central_win7_ix_test-mozmill-bm119-tests1-windows-build4.txt.gz INFO - SUMMARY-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\migration-to-rdf-ui-2\test-migrate-to-rdf-ui-2.js | test-migrate-to-rdf-ui-2.js::test_width_persisted INFO - EXCEPTION: The width of the folderPaneBox was not persisted.: '500' != '200'. INFO - at: test-folder-display-helpers.js line 2890 INFO - assert_true test-folder-display-helpers.js:2890 11 INFO - assert_equals test-folder-display-helpers.js:2877 3 INFO - test_width_persisted test-migrate-to-rdf-ui-2.js:50 3 M-C: Last good: 2d20b365eee19434657f6b365d310e8b70 M-C: First bad: c4e74cfbf7e9d8e297e214478d25e3456f Range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2d20b365eee19434657f6b365d310e8b70&tochange=c4e74cfbf7e9d8e297e214478d25e3456f Looks like bug 1368567: https://hg.mozilla.org/mozilla-central/rev/95ca02ed62c4 Ehsan and Florian, looks like you changes causes us a test failure. I'll confirm by running the test locally.

Locally I'm at 2d20b365eee19434657f6b365d310e8b70 and the test passes. I've imported https://hg.mozilla.org/mozilla-central/rev/95ca02ed62c4 and now the test fails as stated above. For your reference, you can find the test here: https://dxr.mozilla.org/comm-central/rev/291cefb09dff00a327062580caf17c1830d4a50b/mail/test/mozmill/migration-to-rdf-ui-2/test-migrate-to-rdf-ui-2.js#42 Is our test invalid, do we need to adapt it or should bug 1368567 simply be backed out?

Sorry, now the penny dropped when reading bug 1368567 comment #11. This is about localstore.rdf which was discontinued. So I guess the test is now invalid. Aceman, just remove it or replace it with something else, for example instead of using localstore.rdf, we use a pre-canned xulstore.json.

Disabled failing test for now: https://hg.mozilla.org/comm-central/rev/b8876205fa8dbf22f34ffffadce627327ad51f24

Yes, if they removed reading in localstore.rdf then it is useless having the file in our tests (there are also the migration-to-rdf-ui-3 and migration-to-rdf-ui-5 ones). I'll think about what to do here. It is interesting the other tests still pass. Maybe they are not really testing anything, or they incidentally also pass with xulstore defaults.

Any test depending on localstore.rdf needs to be updated, yes.

I have played with this and TB starts with mail.ui-rdf.version = 0. So it does all the migrations defined in mailMigrator.js and the tests check that particular migrations were done (as written in the test headers). If you can convert the localstore.rdf contents to xulstore.json, the tests may still be useful and working.

Hmm, I was hoping you would take the bug. I tried using xulstore.json with this content only, and it didn't work, the 334 didn't seem to be read from the file. {"chrome://messenger/content/messenger.xul":{"mail-toolbar-menubar2":{"autohide":"true"},"qfb-boolean-mode":{"value":"OR"},"header-view-toolbox":{"mode":"full","iconsize":"small","labelalign":"end"},"header-view-toolbar":{"iconsize":"small"},"attachment-view-toolbox":{"mode":"full","iconsize":"small","labelalign":"end"},"attachment-view-toolbar":{"iconsize":"small"},"titlebar-placeholder-on-TabsToolbar-for-captions-buttons":{"width":"100"},"messengerWindow":{"screenX":"0","screenY":"0","width":"1024","height":"994","sizemode":"normal"},"folderPaneBox":{"width":"334"}}} All one line, extracted from a new profile. If we can't make the test useful, we should just delete it altogether.

Aceman, what are we going to do with this? Just remove the test and close the bug?

(In reply to :aceman from comment #6) > I have played with this and TB starts with mail.ui-rdf.version = 0. So it > does all the migrations defined in mailMigrator.js Actually I do not understand why we start with mail.ui-rdf.version = 0 and not with the current UI value. In a clean profile, you won't find and old localstore.rdf or xulstore.json file with a matching pref file with old UI value that needs migration. Or do will you? Then running all the code from UI 0 to current value (15) in https://dxr.mozilla.org/comm-central/rev/952762964f408e33d060dd1110cac0626ae5fa51/mail/base/modules/mailMigrator.js#102 is basically useless. (In reply to Jorg K (:jorgk, GMT+1) from comment #2) > Aceman, just remove it or replace it with something else, for example > instead of using localstore.rdf, we use a pre-canned xulstore.json. We could covert the file to xulstore.json and the test could work. But it seems to me this would test a fictional scenario. Support for xulstore.json was added in https://hg.mozilla.org/comm-central/rev/8d1c850ff12b (in 2014) when UI level was at 5 (seen in the patch). Since then I understand all state in a profile that had the app started was migrated from previous localstore.rdf to xulstore.json and the current UI level was stored and localstore.rdf hasn't been touched since (I can see it in my production profile, it is dated 2015, I can probably just remove it). Till https://hg.mozilla.org/mozilla-central/rev/95ca02ed62c4 if you started the app and you had an old localstore.rdf (any level) it was migrated to xulstore.json. After https://hg.mozilla.org/mozilla-central/rev/95ca02ed62c4 we no longer load in localstore.rd even if it exists. Then either there is a xulstore.json existing or none and we start with clean state (as new profile), but the prefs still store the UI version from the previous run of the app. Even if the stored UI level is old (like 0-5), we have nothing to migrate. If there is a xulstore.json found, the UI level must be 5 or higher. So my theory is you can't ever find a xulstore.json with prefs saying it is an UI level below 5 (localstore is ignored). So I think there is no sense testing migrating xulstore.json from an UI level below 5. And all the 3 test folders could be removed. I'd think we could also remove all code for UI levels below 6 from https://dxr.mozilla.org/comm-central/rev/952762964f408e33d060dd1110cac0626ae5fa51/mail/base/modules/mailMigrator.js#102 . Magnus, can you follow/confirm the theory? Notice Firefox landed xulstore.json at UI level 23 (https://hg.mozilla.org/mozilla-central/rev/25c918c5f3e1) and now they are at 57. You can see at https://dxr.mozilla.org/comm-central/rev/371e44e0034771ec8a5ac3c5a6518ef608227b99/mozilla/browser/components/nsBrowserGlue.js#1743 they have removed code for migrating old UI levels since, up to 14 (but not up to the 23).

(In reply to :aceman from comment #10) > So my theory is you can't ever find a xulstore.json with prefs saying it is > an UI level below 5 (localstore is ignored). So I think there is no sense > testing migrating xulstore.json from an UI level below 5. And all the 3 test > folders could be removed. I don't understand most of this. "And all the 3 test folders could be removed." Which folders? Or do you mean to remove the three tests in test-migrate-to-rdf-ui-2.js which would come to removing the file altogether? The test I had in mind was to use a pre-canned xulstore.json with "folderPaneBox":{"width":"334"} and check that the item is really 334px wide. However, that wouldn't be a migration test, that would check that xulstore.json actually works. I guess that's covered elsewhere in M-C (without having looked).

All 3 test folders: migration-to-rdf-ui-2 migration-to-rdf-ui-3 migration-to-rdf-ui-5 I would make the patch if Magnus (or somebody agrees with the plan)

Interesting, so remove the tests test-migrate-to-rdf-ui-*.js and their respective folders. 3 and 5 still seem to test the integrity of the UI, but not as a result of a migration process. Maybe we should merge the useful (sub)tests into a test-basic-ui.js where we can still check that, for example, the app menu is present (from 5).

(In reply to Jorg K (:jorgk, GMT+1) from comment #13) > 3 and 5 still seem to test the integrity of the UI, but not as a result of a > migration process. Why do you think? See e.g. migration-to-rdf-ui-3. It checks that "qfb-show-filter-bar" is NOT in "tabbar-toolbar". But looking into the localstore.rdf, it is there. So before the test code runs, migration runs and shuffles the elements around. If my theory is true, you can't have a xulstore.json that would have "qfb-show-filter-bar" in "tabbar-toolbar" as any xulstore.json must be at least UI level 5. This particular migration only happens for level < 3. > Maybe we should merge the useful (sub)tests into a > test-basic-ui.js where we can still check that, for example, the app menu is > present (from 5). Maybe you could if you want, but why test things that are initially there via XUL file. If there is a test that exercises the appmenu, that one could test if it is at the right place. But what is the right place? The user can move the appmenu anywhere and everything should still work. So what would your test test? Would it spot if we suddenly make the appmenu on the left as initial state? Why would we want to detect it?

(In reply to :aceman from comment #10) > Magnus, can you follow/confirm the theory? Sounds correct to me.

This removes the tests and the migration code for low UI levels. Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5dc589de42972bcb4f77cc06a3a47c7fe3692780

Pushed by acelists@atlas.sk: https://hg.mozilla.org/comm-central/rev/c2bfe9667d82 remove mozmill/migration-to-rdf-ui-* tests as they are no longer useful and remove UI levels below 5 from XUL store migration as they are no longer possible to be hit. r=mkmelin

Thanks.

Typo: https://hg.mozilla.org/comm-central/rev/c2bfe9667d82e4bf2ea04e07762140dac8a4ed4c#l1.118

Backout by acelists@atlas.sk: https://hg.mozilla.org/comm-central/rev/32a2cee5b071 Backed out changeset c2bfe9667d82 for causing a test failure (bug 1416549). rs=backout

As we know, this makes MozMill startup-firstrun/test-menubar-collapsed.js fail, see bug 1416549 comment #1. The trick to set the menubar to autohide and persist that was introduced in to the mail migrator in bug 791311 here: https://hg.mozilla.org/comm-central/rev/5241f7deed34#l2.60 to fix a regression from bug 650170. We could either leave the code in the mail migrator or try adding it to the XUL file. Let's see.

That's you own suggestion from bug 1416549 comment #1. I'll f? Richard since he might know the history of bug 791311 and bug 650170.

That needed more clean-up from the quirky old past. Surely it makes a whole lot of sense to test what's happening on a new profile if the pref isn't at its standard value. Well, hold on, maybe we can't do that. Some enterprises might have that set in the user prefs to get their users a menubar when they first start?? In this case we need to leave the step in the profile migration. Aceman, what you you think?

Comment on attachment 9011234 [details] [diff] [review] 1371898-autohide-persist.patch Magnus, the patch you approved caused a test failure. New profiles didn't have their menu bar hidden. Two solutions to this: We don't rip out that code from the mail migrator or we add the autohide/persist to the XUL, but then it's not configurable any more via a pref that someone might have as a user pref. What's your opinion? Rip it all out?

Can we keep that code outside the UI version checks? Can it safe to run at each TB start?

Mac has no menubar that can autohide. You should add around the |ïautohide="true" persist="autohide"| a |#ifdef MENUBAR_CAN_AUTOHIDE| (I'm not sure this works out of the box in TB) or move the line inside the |#ifndef XP_MACOSX|.

(In reply to :aceman from comment #26) > Can we keep that code outside the UI version checks? Can it safe to run at > each TB start? Now it only runs on new profiles setting the autohide/persist. Why do it more often?

OK, we could have a way to detect new profiles and only run it once. When we decide to set new profiles to the current UI version and not run any migration. We should file a new bug for that.

(In reply to :aceman from comment #29) > We should file a new bug for that. Indeed, and finish this one here with the material we have. Let's see Magnus' reply and then we land both patches or reduce the first one to leave that step (and move it elsewhere later).

Comment on attachment 9011234 [details] [diff] [review] 1371898-autohide-persist.patch Review of attachment 9011234 [details] [diff] [review]: ----------------------------------------------------------------- So that means I would rather not remove all of this.

OK, let's go with this then. I restored a bit of the code removed in the first patch.

Didn't refresh :-(

Comment on attachment 9011247 [details] [diff] [review] 1371898.patch (JK) Review of attachment 9011247 [details] [diff] [review]: ----------------------------------------------------------------- Yes, this looks usable now and we fix the "new profile" check in a new bug.

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/e1b439708ed0 remove mozmill/migration-to-rdf-ui-* tests as they are no longer useful and remove UI levels below 4. r=mkmelin,jorgk

This this was initially planned for TB 58 we should consider removing the defunct processing from the ESR.

Late to the party but yes I agree this is all worth ripping out. Probably we should remove even more levels too. I don't recall exactly which version you need to upgrade to in between, not to risk messing up mail indexes. IIRC it's v17. There's not really a proper upgrade v2 -> 60. So then all the ones < v17 should go.

(In reply to Magnus Melin [:mkmelin] from comment #37) > Late to the party but yes I agree this is all worth ripping out. Even the mail.main_menu.collapse_by_default pref and all that goes with it, see attachment 9011234 [details] [diff] [review]? In this case, un-obsolete it and put an f+ onto it, please. It's not quite up-to-date, some code from the mail migrator would also need to go now, the one I left: https://hg.mozilla.org/comm-central/rev/e1b439708ed0#l1.116 We can do it in a new bug.

No what was done was fine. I like the pref. (Or well, not the value, I'd rather have menus on by default, but that's another story.)

Magnus, please comment further in bug 1493513. I'm glad we settled this bug here, since it looks like going further is not 100% straight forward.

TB 60.1/60.2: https://hg.mozilla.org/releases/comm-esr60/rev/498a2e30c4614ee5401617114113cd56f3016112

Beta (TB 63): https://hg.mozilla.org/releases/comm-beta/rev/352500ee7f811c4982d49967ecfd6fb638358dd0