1460232 - Port bug 1352539: to TB: Build error: Deleting file '.deps/generated_en-US/list.json' and 10 Xpcshell test failures

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

https://taskcluster-artifacts.net/KN-Qc_TuTX2pB73NxKKUJQ/0/public/logs/live_backing.log

[task 2018-05-09T09:44:50.364Z] 09:44:50     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-thunderbird/comm/mail/locales'
[task 2018-05-09T09:44:50.364Z] 09:44:50     INFO -  make[4]: *** Deleting file '.deps/generated_en-US/list.json'
[task 2018-05-09T09:44:50.364Z] 09:44:50     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-thunderbird/comm/mail/locales'
[task 2018-05-09T09:44:50.364Z] 09:44:50     INFO -  /builds/worker/workspace/build/src/obj-thunderbird/_virtualenvs/init/bin/python -m mozbuild.action.generate_searchjson /builds/worker/workspace/build/src/comm/mail/locales/search/list.json en-US .deps/generated_en-US/list.json
[task 2018-05-09T09:44:50.364Z] 09:44:50     INFO -  Error: Missing default searchDefault in list.json
[task 2018-05-09T09:44:50.364Z] 09:44:50     INFO -  Makefile:88: recipe for target '.deps/generated_en-US/list.json' failed
[task 2018-05-09T09:44:50.364Z] 09:44:50     INFO -  make[4]: *** [.deps/generated_en-US/list.json] Error 1
[task 2018-05-09T09:44:50.364Z] 09:44:50     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-thunderbird/comm/mail/locales'
[task 2018-05-09T09:44:50.364Z] 09:44:50     INFO -  make[4]: *** Waiting for unfinished jobs....
[task 2018-05-09T09:44:50.364Z] 09:44:50     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-thunderbird/comm/mail/locales'

M-C last good: 7c83ceac4be6d055bebd870a82b78b76de
M-C first bad: 0cd106a2eb78aa04fd481785257e6f4f9b
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7c83ceac4be6d055bebd870a82b78b76de&tochange=0cd106a2eb78aa04fd481785257e6f4f9b

Comment 1

[Richard Marti (:Paenglab)]

This could be from bug 1352539.

Comment 2

[Jorg K (CEST = GMT+2)]

Looks like bug 1352539:
https://hg.mozilla.org/mozilla-central/rev/f8a023df39e2

Florian, Michael, what do we need to do here, all our builds are busted :-(

Comment 3

[Jorg K (CEST = GMT+2)]

Oh, at times it helps to read the message ;-)

Missing default searchDefault in list.json

Could be this:
https://hg.mozilla.org/mozilla-central/rev/f8a023df39e2#l3.6
+    "searchDefault": "Google",

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 1460232-searchDefault.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4b0b6140cdd05649d837be5369693fb04c221d81

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 1460232-searchDefault.patch (v2)

More comprehensive port.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f995a5cf67a597c1f6fde860c5eb35586c694f83

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: 1460232-searchDefault.patch (v3)

Missed one line.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7eb9d52466f3
Port bug 1352539:  Move default search engine to list.json (add searchDefault). rs=bustage-fix

Comment 8

[Jorg K (CEST = GMT+2)]

OK, this gives us a working build, but we get heaps of test failures as can me seen on the try runs:

TEST-UNEXPECTED-FAIL | toolkit/components/places/tests/unifiedcomplete/test_PlacesSearchAutocompleteProvider.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | toolkit/components/search/tests/xpcshell/test_list_json_searchdefault.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | toolkit/components/search/tests/xpcshell/test_selectedEngine.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | toolkit/components/search/tests/xpcshell/test_currentEngine_fallback.js | xpcshell return code: 0
TypeError: Services.search.currentEngine is null at /Users/cltbld/tasks/task_1525863775/build/tests/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_currentEngine_fallback.js:22
TEST-UNEXPECTED-FAIL | toolkit/components/search/tests/xpcshell/test_geodefaults.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | toolkit/components/search/tests/xpcshell/test_geodefaults.js | no_request_if_prefed_off - [no_request_if_prefed_off : 74] "Bing" == "bing"
TEST-UNEXPECTED-FAIL | toolkit/components/search/tests/xpcshell/test_addEngineWithDetails.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | toolkit/components/search/tests/xpcshell/test_addEngineWithDetailsObject.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | toolkit/components/search/tests/xpcshell/test_searchReset.js | xpcshell return code: 0

Michael, what are we missing?

Comment 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 8974342 [details] [diff] [review]
1460232-searchDefault.patch (v3)

Also, what do these lines do:
https://hg.mozilla.org/mozilla-central/rev/f8a023df39e2#l3.25
+      },
+      "KZ": {
+        "searchDefault": "Яндекс"
+      },
Do we need to add something like this as well?

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: 1460232-follow-up.patch

Oh, I see, you added |"searchDefault": "Google"| with an upper-case "Google". Let me try with an upper-case "Bing". I've also added default Google where there wasn't a "bing" in the list:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=47364f20f5815d227e8651f664684c2ccb297592

Comment 11

[Jorg K (CEST = GMT+2)]

OK, with the follow-up we're down to two failures:
TEST-UNEXPECTED-FAIL | toolkit/components/search/tests/xpcshell/test_list_json_searchdefault.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | toolkit/components/search/tests/xpcshell/test_selectedEngine.js | xpcshell return code: 0

I'll take a look locally.

Comment 12

[Mike Kaply [:mkaply]]

Comment on attachment 8974342 [details] [diff] [review]
1460232-searchDefault.patch (v3)

Sorry, completely forgot about Thunderbird.

This is correct with the "Bing" you did in the other patch.

Comment 13

[Jorg K (CEST = GMT+2)]

OK, thanks, but with those two patches applied, we still have two failures:
TEST-UNEXPECTED-FAIL | toolkit/components/search/tests/xpcshell/test_list_json_searchdefault.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | toolkit/components/search/tests/xpcshell/test_selectedEngine.js | xpcshell return code: 0

Running the first one locally gives a bunch of output and ends with:

 0:01.33 INFO "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "US"" {file: "c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/_tests/xpcshell/toolkit/components/search/tests/xpcshell/head_search.js" line: 249}]"

That code is:
  if (isUS && ("searchDefault" in searchSettings.US)) {
    defaultEngineName = searchSettings.US.searchDefault;
  }
  return defaultEngineName;

So somehow, searchSettings, which comes from searchSettings = parseJsonFromStream(chan.open2()); doesn't have a US property.

The second test test_selectedEngine.js fails at
 0:01.41 FAIL test_defaultEngine - [test_defaultEngine : 13] "Bing" == "undefined"
c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/_tests/xpcshell/toolkit/components/search/tests/xpcshell/test_selectedEngine.js:test_defaultEngine:13

So that looks like some inconsistency in the JSON file.

Comment 14

[Mike Kaply [:mkaply]]

It shouldn't need a US property. We explicitly check for that. 

I'll debug this and get you a fix.

Comment 15

[Mike Kaply [:mkaply]]

I just checked out thunderbird and you didn't apply the second patch. searchDefault is "bing"

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/16c11d312c0a
Follow-up: Fix spelling of default search engine and provide localised default where global default is not available. r=mkaply DONTBUILD

Comment 17

[Jorg K (CEST = GMT+2)]

I've landed the second patch now so we're all on the same page.

Please run the failing tests:
mach xpcshell-test toolkit/components/search/tests/xpcshell/test_selectedEngine.js
mach xpcshell-test toolkit/components/search/tests/xpcshell/test_list_json_searchdefault.js

I noticed this:
    "ja": {
      "default": {
        "searchDefault": "Google",
        "visibleDefaultEngines": [
          "google-jp", "yahoo-jp", "amazon-jp", "rakuten", "yahoo-jp-auctions", "oshiete-goo", "twitter-ja", "wikipedia-ja"
        ]
      }
    },

So the default isn't in the list, and FF has "google" in the list instead of "google-jp". But using "google-jp" or removing "ja" entirely doesn't fix the test failure either.

Comment 18

[Mike Kaply [:mkaply]]

google-jp still has an internal name of Google, so that's fine:

https://dxr.mozilla.org/l10n-central/source/ja-JP-mac/mail/searchplugins/google-jp.xml

I'm building Thunderbird now and will be able to take a look in a few minutes.

Comment 19

[Francesco Lodolo [:flod]]

"searchDefault" uses the ShortName element, which is "Google" for google-jp.xml
https://hg.mozilla.org/l10n-central/ja/file/tip/mail/searchplugins/google-jp.xml#l6

The same is true for search.order. On the other hand, visibleDefaultEngines uses file names, minus the extension (".xml").

Hopefully we can get to some uniformity once everything is centralized. Historically, they were in two different places (list.txt, region.properties). Not that it would make things less confusing.

Comment 20

[Mike Kaply [:mkaply]]

Attachment: Fix test to account for no US locale

So the issue is that you don't have US at all which I didn't expect.

Fixing the test is the right thing. I'll submit a formal mozreview in a bit.

Comment 21

[Jorg K (CEST = GMT+2)]

Thanks, did you have a chance to see why
mach xpcshell-test toolkit/components/search/tests/xpcshell/test_selectedEngine.js
fails. Or any hint how to debug this? More details in comment #13.

Comment 22

[Jorg K (CEST = GMT+2)]

Running
  mach xpcshell-test toolkit/components/search/tests/xpcshell/test_list_json_searchdefault.js
with the patch applied gives a different failure:

 0:01.44 pid:9496 *** Search: NOTIFY: Engine: "Amazon.com"; Verb: "engine-current"
 0:01.44 pid:9496 [9496, LoadRoots] WARNING: This method is lossy. Use GetCanonicalPath !: file c:/mozilla-source/comm-central/xpcom/io/nsLocalFileWin.cpp, line 3378
 0:01.45 FAIL test_searchDefaultEngineUS - [test_searchDefaultEngineUS : 22] expected US default search engine - "Amazon.com" == "undefined"
c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/_tests/xpcshell/toolkit/components/search/tests/xpcshell/test_list_json_searchdefault.js:test_searchDefaultEngineUS:22

which appears to be similar to the failure of test_selectedEngine.js:
toolkit/components/search/tests/xpcshell/test_selectedEngine.js
  FAIL test_defaultEngine - [test_defaultEngine : 13] "Bing" == "undefined"
c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/_tests/xpcshell/toolkit/components/search/tests/xpcshell/test_selectedEngine.js:test_defaultEngine:13
  FAIL toolkit/components/search/tests/xpcshell/test_selectedEngine.js - xpcshell return code: 0

Can you spot anything wrong in our list.json?

Comment 23

[Mike Kaply [:mkaply]]

With my change to head_search.js applied, both tests pass.

Comment 24

[Francesco Lodolo [:flod]]

Is the empty regionOverrides needed in list.json? As far as I see, the code always check if that key is available, doesn't assume it is.

Comment 25

[Jorg K (CEST = GMT+2)]

Hmm, not for me, but let me rebuild with M-C at tip. My build doesn't contain the last merge, but that doesn't have anything search related.

Comment 26

[Mike Kaply [:mkaply]]

regionOverrides is read at build time, so it wouldn't cause a test failure.

Comment 27

[Francesco Lodolo [:flod]]

(In reply to Mike Kaply [:mkaply] from comment #26)
> regionOverrides is read at build time, so it wouldn't cause a test failure.

Sorry, there was a question unrelated to the test failure. Is it needed at all?

Comment 28

[Mike Kaply [:mkaply]]

No, regionOverrides is optional.

Comment 29

[Jorg K (CEST = GMT+2)]

Sorry, both tests still fail for me locally. Here's a try build with your change included:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9c108fe8d5645e7b28be62986fd02c0d93e3f268

The trick is that I pointed the M-C directory to use try to where I've pushed this:
https://hg.mozilla.org/try/rev/adc646ffd176cf31f68a9952057b2d314af11654

Comment 30

[Jorg K (CEST = GMT+2)]

Without your patch Aceman did a try run here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6672837a190a805ff7ea19706a76aa5f1ff37c48
On Mac he got two failures:
TEST-UNEXPECTED-FAIL | toolkit/components/search/tests/xpcshell/test_list_json_searchdefault.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | toolkit/components/search/tests/xpcshell/test_selectedEngine.js | xpcshell return code: 0
and on Linux only one:
TEST-UNEXPECTED-FAIL | toolkit/components/search/tests/xpcshell/test_list_json_searchdefault.js | xpcshell return code: 0

I guess that's the one you've fixed. Which platform are you on?

Comment 31

[Mike Kaply [:mkaply]]

I'm on Mac. Nothing about this should be platform specific though. Waiting for your try run so I can see.

Comment 32

[Jorg K (CEST = GMT+2)]

Check
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=28fb09316488db06ca1f606429a75c29d0bd856d
and click the X's to see that the platforms are different.

Comment 33

[Jorg K (CEST = GMT+2)]

According to
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9c108fe8d5645e7b28be62986fd02c0d93e3f268
your fix worked. So I don't know what's going on locally.

Maybe we just land that and see.

Comment 34

[Mike Kaply [:mkaply]]

Attachment: Bug 1460232 - Update search test to handle missing region.

Review commit: https://reviewboard.mozilla.org/r/242862/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242862/

Comment 35

[Drew Willcoxon :adw]

Comment on attachment 8974523 [details]
Bug 1460232 - Update search test to handle missing region.

https://reviewboard.mozilla.org/r/242862/#review248760

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/117b148d7a02
Update search test to handle missing region. r=adw

Comment 37

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/117b148d7a02

Comment 38

[Mike Kaply [:mkaply]]

Is this all working now?

Comment 39

[Jorg K (CEST = GMT+2)]

Yes, thanks for your help. I'm not sure why I had local failures even with your patch. Does the test depend on the locale of the machine it runs on? That would explain why it passes on the en-US servers and not locally.

Comment 40

[Mike Kaply [:mkaply]]

> Yes, thanks for your help. I'm not sure why I had local failures even with your patch. Does the test depend on the locale of the machine it runs on? That would explain why it passes on the en-US servers and not locally.

It shouldn't, but I guess that's possible.