1276720 - browser chrome tests fail on ESR45 due to unexpected Firefox version

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Rail Aliiev [:rail]]

See https://treeherder.mozilla.org/logviewer.html#?job_id=45883&repo=mozilla-esr45#L4264

19:02:55 INFO - 829 INFO TEST-UNEXPECTED-FAIL | toolkit/mozapps/extensions/test/browser/browser_bug590347.js | Warning message should be correct - Got Test add-on is incompatible with Firefox 45.1.2., expected Test add-on is known to cause security or stability issues. 

I landed https://hg.mozilla.org/releases/mozilla-esr45/rev/6f20e58674c6 to start using real version numbers on ESR, so we can promote CI builds to release builds. This also matches what we had in release builds before: https://hg.mozilla.org/releases/mozilla-esr45/rev/8b70c093909e

Sounds like this breaks some assumptions in the tests (they expect "esrpre" or something?)

Comment 1

[Rail Aliiev [:rail]]

Dave, maybe you have ideas how to fix this?

Comment 2

[Rail Aliiev [:rail]]

Something tells me that we should patch https://hg.mozilla.org/releases/mozilla-esr45/file/tip/toolkit/mozapps/extensions/test/browser/head.js#l50

Comment 3

[Rail Aliiev [:rail]]

Attachment: MozReview Request: Bug 1276720 - Add esr to checkCompatibility list; r=Gijs

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

Comment 4

[Rail Aliiev [:rail]]

To clarify the issue: in bug 1271832 we switched to mozharness based builds and in bug 1253757 we changed the update channel to "esr" for all opt CI builds.

PREF_CHECK_COMPATIBILITY set in https://hg.mozilla.org/releases/mozilla-esr45/file/tip/toolkit/mozapps/extensions/test/browser/head.js#l57 and uses Firefox version in the key name:

PREF_CHECK_COMPATIBILITY = "extensions.checkCompatibility." + version;

Fore release channel (aurora/beta/release) version value is based on app version, otherwise it's set to "nighlty": https://hg.mozilla.org/releases/mozilla-esr45/file/tip/toolkit/mozapps/extensions/test/browser/head.js#l53

The test uses PREF_CHECK_COMPATIBILITY in https://hg.mozilla.org/releases/mozilla-esr45/file/tip/toolkit/mozapps/extensions/test/browser/browser_bug590347.js#l98, but fails to set it properly if the update channel is set to "esr".

Comment 5

[Justin Wood (:Callek)]

Comment on attachment 8758017 [details]
MozReview Request: Bug 1276720 - Add esr to checkCompatibility list; r=Gijs

Looks trivial enough, r+ for landing, but leaving existing request open so a proper peer can give it a once-over.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8758017 [details]
MozReview Request: Bug 1276720 - Add esr to checkCompatibility list; r=Gijs

https://reviewboard.mozilla.org/r/56378/#review52958

rs=me for the trivial fix here, please also land on m-c so we don't break this next ESR.


However... have we used this update channel in the past? Like for esr38? If not, I wonder if other code expects it...

Comment 7

[Rail Aliiev [:rail]]

https://reviewboard.mozilla.org/r/56378/#review52958

not on esr38, this is something new I landed yesterday (the channel switch from default to esr)

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/09d0d424f914

Comment 9

[Rail Aliiev [:rail]]

Comment on attachment 8758017 [details]
MozReview Request: Bug 1276720 - Add esr to checkCompatibility list; r=Gijs

This is to fix some browser chrome tests, behaving differently after we started using "esr" update channel in CI builds. The fix is trivial and the risk is very low.

User impact if declined: None
Fix Landed on Version: 49
Risk to taking this patch (and alternatives if risky): this is a test-only patch, the risk is minimal
String or UUID changes made by this patch: None

Comment 10

[Rail Aliiev [:rail]]

NI is not required anymore I think.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/09d0d424f914

Comment 12

[Liz Henry (:lizzard)]

Comment on attachment 8758017 [details]
MozReview Request: Bug 1276720 - Add esr to checkCompatibility list; r=Gijs

Test only fix, ok to uplift to esr45

Comment 13

[Rail Aliiev [:rail]]

Comment on attachment 8758017 [details]
MozReview Request: Bug 1276720 - Add esr to checkCompatibility list; r=Gijs

https://hg.mozilla.org/releases/mozilla-esr45/rev/4f4d27edc880

Comment 14

[Rail Aliiev [:rail]]

https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr45&revision=4f4d27edc880411a4286e04cb4dde35ab14273a4 lgtm!