Closed Bug 981251 Opened 10 years ago Closed 10 years ago

Test app uninstallation

Categories

(Firefox Graveyard :: Webapp Runtime, defect, P1)

x86_64
Linux
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 33

People

(Reporter: marco, Assigned: marco)

References

Details

Attachments

(1 file, 1 obsolete file)

It should be easy on Linux, I'm not sure how to do it on Windows or Mac.
Priority: -- → P1
Depends on: 774144
Depends on: 991766
Assignee: nobody → mar.castelluccio
Status: NEW → ASSIGNED
Depends on: 994834
Depends on: 993690
Attached patch Patch (obsolete) — Splinter Review
Depends on the patches in the other bugs.
The test is a bit more complex for Mac, because Mac has a weird behavior (PathForAppWithIdentifier returns the app path until you try to launch it. The launch fails and then PathForAppWithIdentifier returns false).

Sent to try with the other patches, and it's green:
https://tbpl.mozilla.org/?tree=Try&rev=60cf9c4b6525
Attachment #8405862 - Flags: review?(myk)
Comment on attachment 8405862 [details] [diff] [review]
Patch

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

::: toolkit/webapps/tests/head.js
@@ +43,5 @@
>      return true;
>    });
>  }
>  
> +function dirContainsOnly(dir, files) {

Nit: this would be simpler if you first slurped the list of files in the directory:

  let filesInDir = [file for each (file in iterator)];

Then you would be able to use use Array.filter to generate arrays of unexpectedly found and missing items:

  let unexpectedlyFoundInDir = filesInDir.filter(v => files.indexOf(v) == -1);
  let unexpectedlyMissingFromDir = files.filter(v => filesInDir.indexOf(v) == -1);

@@ +69,5 @@
> +    }
> +
> +    // If one or more files aren't present in the directory, return false.
> +    if (files.length > 0) {
> +      return false;

Nit: since you report files that were unexpectedly found in the directory via the info() call in the iterator loop above, it'd make sense to also report here files that are unexpectedly missing from the directory.

::: toolkit/webapps/tests/test_hosted_uninstall.xul
@@ +221,3 @@
>    }
> +  // On Mac, the app is moved to the trash, it is still considered launchable
> +  // (because it does have a install path).

Nit: that's not quite right, as you can't actually launch an app in the trash (which you test below).  So we should fix WebappOSUtils.isLaunchable to report trashed apps as unlaunchable, even before a failure to launch it.  But we can do that in a followup bug.

@@ +224,5 @@
> +  if (!isMac) {
> +    while (WebappOSUtils.isLaunchable(app)) {
> +      yield wait(1000);
> +    }
> +    ok(true, "App not launchable");

Isn't Mac the only platform on which isLaunchable is asynchronous (and therefore potentially out-of-date)?  I thought Windows and Linux both immediately reflect changes to an app's installation status.

@@ +229,5 @@
>  
> +    is(WebappOSUtils.getInstallPath(app), null, "getInstallPath == null");
> +  } else {
> +    trashDir = WebappOSUtils.getInstallPath(app);
> +    ok(trashDir.contains(".Trash"), "App moved to Trash");

Nit: in a followup, we should add the user's trash dir to OS.Constants.Path, so we can test it more precisely here.
Attachment #8405862 - Flags: review?(myk) → review+
Attached patch PatchSplinter Review
Attachment #8405862 - Attachment is obsolete: true
Attachment #8410972 - Flags: review+
Keywords: checkin-needed
Backed out for making bug 763894 perma-fail.
https://hg.mozilla.org/integration/mozilla-inbound/rev/621bb79845e3

https://tbpl.mozilla.org/php/getParsedLog.php?id=38328630&tree=Mozilla-Inbound

Some observations:
1) These new tests run right before the now-perma-failing test. Are these tests somehow still doing work after they finish?
2) Can't help but notice that every other test in this dir is skipped on ASAN builds. Should these too?
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #5)
> 1) These new tests run right before the now-perma-failing test. Are these
> tests somehow still doing work after they finish?

Yes, this is most probably the issue, the cleanup function is asynchronous.

> 2) Can't help but notice that every other test in this dir is skipped on
> ASAN builds. Should these too?

Only the tests that actually launch an app need to be skipped on ASAN (test_hosted_launch and test_packaged_launch).
Depends on: 1000512
(In reply to Marco Castelluccio [:marco] from comment #6)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #5)
> > 1) These new tests run right before the now-perma-failing test. Are these
> > tests somehow still doing work after they finish?
> 
> Yes, this is most probably the issue, the cleanup function is asynchronous.

I've tried making the cleanup function synchronous, but the other test is still failing (only on Windows XP).
Myk, I'd like to land the tests disabled only on Windows XP (since it's the only problematic platform). Do you think it's OK?
Flags: needinfo?(myk)
(In reply to Marco Castelluccio [:marco] from comment #8)
> Myk, I'd like to land the tests disabled only on Windows XP (since it's the
> only problematic platform). Do you think it's OK?

Yes, provided we open a bug about the issue with the tests on Windows XP.
Flags: needinfo?(myk)
https://hg.mozilla.org/mozilla-central/rev/3b49cfc3c734
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.