Closed Bug 1058182 Opened 7 years ago Closed 7 years ago

Fix app update xpcshell tests due to the Mac v2 bundle structure.

Categories

(Toolkit :: Application Update, defect)

x86_64
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox34 --- fixed
firefox35 --- fixed

People

(Reporter: robert.strong.bugs, Assigned: robert.strong.bugs)

References

Details

Attachments

(1 file, 22 obsolete files)

606.31 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
No description provided.
Attached patch patch in progress (obsolete) — Splinter Review
OS: Windows 8.1 → Mac OS X
Attached patch patch in progress (obsolete) — Splinter Review
This will hopefully fix the current set of xpcshell failures
Attachment #8478531 - Attachment is obsolete: true
Attached patch patch in progress (obsolete) — Splinter Review
This should fix the remaining two app update xpcshell failures.
Attachment #8483740 - Attachment is obsolete: true
Since I don't have all of the xpchsell harness changes I split out a separate patch for the xpchsell harness changes.
Attachment #8483829 - Attachment is obsolete: true
lol s/xpchsell/xpcshell/g
Attachment #8485996 - Attachment description: patch with xpcshell harness changes → patch xpcshell harness changes
Attachment #8486285 - Attachment description: patch in progress without xpcshell harness changes → 1. patch in progress without xpcshell harness changes
Attachment #8485996 - Attachment description: patch xpcshell harness changes → 2. patch xpcshell harness changes
Attached patch Test patch in progress (obsolete) — Splinter Review
All tests pass except for one on Mac
Attachment #8485996 - Attachment is obsolete: true
Attachment #8486285 - Attachment is obsolete: true
Note: I am going to have to create new mar files and then change how the precomplete file is read in updater.cpp from bug 1059467. Fun times!
Attached patch updated tests - patch (obsolete) — Splinter Review
This gets the tests most of the way there so I'm requesting review. I still need to investigate why the service tests fail and I will need to modify the tests for new mars after I create them which I will do in separate patches. I've also noticed that marStageSuccessComplete.js times out when it runs in parallel but succeeds on the subsequent sequential run which appears to be due to the link file not being created before it is checked.
Attachment #8488534 - Flags: review?(netzen)
Attachment #8487758 - Attachment is obsolete: true
This fixes the timeout due to the link file test
Note: upgrading the old service worked for me locally so sending to try.
Upgrading the service worked on try
https://tbpl.mozilla.org/?tree=Try&rev=761d2d5050cd

I think I know why marAppApplyUpdateStageSuccess.js failed on Linux

There are almost permanent errors on Windows for the following tests where the post update process was launched after staging an update that I need to look into.
marStageSuccessCompleteSvc.js
marStageSuccessPartialSvc.js

All in all things looks pretty good!
Comment on attachment 8488534 [details] [diff] [review]
updated tests - patch

Review of attachment 8488534 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

::: toolkit/mozapps/update/tests/unit_service_updater/marAppApplyDirLockedStageFailureSvc_win.js
@@ +25,5 @@
>  
>    createUpdaterINI(false);
>  
> +  if (IS_WIN) {
> +    Services.prefs.setBoolPref(PREF_APP_UPDATE_SERVICE_ENABLED, true);

Is this the default already? If you're not sure I'm fine with just leaving it.
Attachment #8488534 - Flags: review?(netzen) → review+
(In reply to Brian R. Bondy [:bbondy] from comment #15)
> Comment on attachment 8488534 [details] [diff] [review]
> updated tests - patch
> 
> Review of attachment 8488534 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, thanks!
> 
> :::
> toolkit/mozapps/update/tests/unit_service_updater/
> marAppApplyDirLockedStageFailureSvc_win.js
> @@ +25,5 @@
> >  
> >    createUpdaterINI(false);
> >  
> > +  if (IS_WIN) {
> > +    Services.prefs.setBoolPref(PREF_APP_UPDATE_SERVICE_ENABLED, true);
> 
> Is this the default already? If you're not sure I'm fine with just leaving
> it.
Thanks for pointing this out. I wanted to protect the service tests in case an app has that set to disabled.
Attachment #8488958 - Attachment is obsolete: true
Attachment #8488959 - Attachment is obsolete: true
Attachment #8489188 - Flags: review?(netzen)
Attached patch patch new test support files (obsolete) — Splinter Review
Attachment #8489189 - Flags: review?(netzen)
Attached patch patch - test changes part 2 (obsolete) — Splinter Review
Attachment #8489191 - Flags: review?(netzen)
Attached patch patch - test changes part 2 (obsolete) — Splinter Review
forgot to update the "patch - test changes part 2" patch
Attachment #8489191 - Attachment is obsolete: true
Attachment #8489191 - Flags: review?(netzen)
Attachment #8489193 - Flags: review?(netzen)
Pushed to try
https://tbpl.mozilla.org/?tree=Try&rev=b4542505e600

I expect that marStageSuccessCompleteSvc.js and marStageSuccessPartialSvc.js will fail and I'm looking into why.
Comment on attachment 8489188 [details] [diff] [review]
patch new binary test support files

unify is failing on the new mac binaries so clearing review request
Attachment #8489188 - Flags: review?(netzen)
Comment on attachment 8489189 [details] [diff] [review]
patch new test support files

This patch will need to be changed too
Attachment #8489189 - Flags: review?(netzen)
Comment on attachment 8489193 [details] [diff] [review]
patch - test changes part 2

Also needs changes :(
Attachment #8489193 - Attachment is obsolete: true
Attachment #8489193 - Flags: review?(netzen)
Attached patch patch - new test mars (obsolete) — Splinter Review
Attachment #8489188 - Attachment is obsolete: true
Attachment #8489215 - Flags: review?(netzen)
Attached patch patch - new test support files (obsolete) — Splinter Review
Attachment #8489189 - Attachment is obsolete: true
Attachment #8489216 - Flags: review?(netzen)
Attached patch patch - test changes part 2 (obsolete) — Splinter Review
Attachment #8489217 - Flags: review?(netzen)
Pushed to try
https://tbpl.mozilla.org/?tree=Try&rev=f8d3ba1d5cff

I expect that marStageSuccessCompleteSvc.js and marStageSuccessPartialSvc.js will fail and I'm looking into why.
Attached patch patch - test support files (obsolete) — Splinter Review
Attachment #8489216 - Attachment is obsolete: true
Attachment #8489216 - Flags: review?(netzen)
Attachment #8489228 - Flags: review?(netzen)
Attached patch patch - test changes part 2 (obsolete) — Splinter Review
missed some changes
Attachment #8489217 - Attachment is obsolete: true
Attachment #8489217 - Flags: review?(netzen)
Attachment #8489230 - Flags: review?(netzen)
marStageSuccessCompleteSvc.js and marStageSuccessPartialSvc.js is failing intermittently and I'm looking into it
Attachment #8489228 - Attachment description: patch - new test support files → patch - test support files
Comment on attachment 8489230 [details] [diff] [review]
patch - test changes part 2

Removed the following unused code locally.
+  // Replace %DIR_RESOURCES% with the platform specific value so the same log
+  // can be used with Mac OS X.
+  compareLogContents = compareLogContents.replace(/%DIR_RESOURCES%/g, DIR_RESOURCES);
Also removed the following debugging code locally
+                                        logTestInfo("compareLogContents\n" + compareLogContents);
+                                        logTestInfo("updateLogContents\n" + updateLogContents);
Attached patch patch - test changes part 2 (obsolete) — Splinter Review
Added a couple of more checks for launching the post update process.
Attachment #8489230 - Attachment is obsolete: true
Attachment #8489230 - Flags: review?(netzen)
Attachment #8489886 - Flags: review?(netzen)
Attachment #8489886 - Flags: review?(netzen) → review+
Attachment #8489215 - Flags: review?(netzen) → review+
Comment on attachment 8489228 [details] [diff] [review]
patch - test support files

Review of attachment 8489228 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for separating out this stuff :)
Attachment #8489228 - Flags: review?(netzen) → review+
Attached patch Combined patch (obsolete) — Splinter Review
Carrying forward r+

I moved the client changes over to the patch in bug 1064523
Attachment #8488534 - Attachment is obsolete: true
Attachment #8489192 - Attachment is obsolete: true
Attachment #8489215 - Attachment is obsolete: true
Attachment #8489228 - Attachment is obsolete: true
Attachment #8489886 - Attachment is obsolete: true
Attachment #8490311 - Flags: review+
Depends on: 1072554
Comment on attachment 8490311 [details] [diff] [review]
Combined patch

Review of attachment 8490311 [details] [diff] [review]:
-----------------------------------------------------------------

Just some drive-by feedback. Stumbled over this during the review for bug 1072554. Would be great if we could include this in the final patch that's going to land on m-c.

::: toolkit/mozapps/update/tests/unit_aus_update/head_update.js
@@ +9,2 @@
>  // MOZ_APP_VENDOR is optional.
>  // On Windows, if MOZ_APP_VENDOR is not defined the updates directory will be

With the removal of the ifdef we should probably update the comment to no longer reference Windows only.

@@ +911,5 @@
>    // adjustGeneralPaths registers a cleanup function that calls end_test when
>    // it is defined as a function.
>    adjustGeneralPaths();
>  
>    // Remove the updates directory on Windows which is located outside of the

Ideally, this comment would mention "Windows and Mac", similar to the new comment in |cleanupTestCommon|.
Thanks and I'll update those comments locally
Comment on attachment 8490311 [details] [diff] [review]
Combined patch

Review of attachment 8490311 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/update/tests/unit_aus_update/head_update.js
@@ +913,5 @@
>    adjustGeneralPaths();
>  
>    // Remove the updates directory on Windows which is located outside of the
>    // application directory after the call to adjustGeneralPaths has set it up.
>    // Since the test hasn't ran yet and the directory shouldn't exist finished

sorry, one more nit since we're already updating this comment: s/ran/run/ and if I'm understanding the comment correctly, I believe the word "finished" should be removed in this sentence.
Attachment #8490311 - Attachment is obsolete: true
Attachment #8494908 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b1d8f769952a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Landed on aurora in the Mac V2 signing combined patch in bug 1047584
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.