770771 - Implement WebappRT unit test framework on Windows

Status: RESOLVED FIXED

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

Description

[Drew Willcoxon :adw]

Currently the WebappRT test framework works only on OS X.  It should be updated to work on Windows too.  I think the only thing that needs to be done is to modify the stub executable to (1) respect the -profile command-line argument and (2) look for the Firefox binary in the same directory as the stub executable.  See the changes to webapprt.mm in bug 733631 attachment 638040 [details] [diff] [review]; the changes to the Windows executable should be similar.

Comment 1

[Marco Castelluccio [:marco]]

Is anyone working on this?
Otherwise, I can take this bug as it's pretty similar to bug 770772.

Comment 2

[Jason Smith [:jsmith]]

(In reply to Marco Castelluccio from comment #1)
> Is anyone working on this?
> Otherwise, I can take this bug as it's pretty similar to bug 770772.

To my knowledge no one is working on this. Feel free to take it.

Comment 3

[Drew Willcoxon :adw]

I was planning on working on it, but I haven't started yet.  So go for it. :)

There's one other small thing I forgot to mention here and in bug 770772.  testing/testsuite-targets.mk needs to be updated to support the webapprt-test-content and webapprt-test-chrome make targets on Windows and Linux.  See the changes to that file in the attachment linked in comment 0.

Comment 4

[Marco Castelluccio [:marco]]

(In reply to Drew Willcoxon :adw from comment #3)
> I was planning on working on it, but I haven't started yet.  So go for it. :)
> 
> There's one other small thing I forgot to mention here and in bug 770772. 
> testing/testsuite-targets.mk needs to be updated to support the
> webapprt-test-content and webapprt-test-chrome make targets on Windows and
> Linux.  See the changes to that file in the attachment linked in comment 0.

Yes, I've already figured that out :)
There's just a small problem with Linux as the webapprt binary isn't in the same directory of the Firefox binary (in the dist/bin folder there's a symlink to the actual webapprt binary).

Comment 5

[Jason Smith [:jsmith]]

We're blocking on this for the 1st release of the desktop webrt, so flagging for tracking.

Comment 6

[Marco Castelluccio [:marco]]

Attachment: WIP

There's still an error that I need to address:

> JS Component Loader: ERROR resource://gre/modules/Webapps.jsm:196
>                     TypeError: ({installOrigin:"http://mochi.test:8888", origin:"http://mochi.test:8888", manifestURL:"http://mochi.test:8888/webapprtChrome/webapprt/test/chrome/sample.webapp", manifest:{name:"Sample Test Webapp", description:"A webapp that demonstrates how to make a WebappRT test.", launch_path:"/webapprtChrome/webapprt/test/chrome/sample.html"}, receipts:[]}) is not extensible

Comment 7

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 8

[Drew Willcoxon :adw]

Comment on attachment 640651 [details] [diff] [review]
Patch

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

Thanks, this basically looks fine to me.  I want to do one more round with my target and profile comments addressed.

::: testing/testsuite-targets.mk
@@ +146,5 @@
> +	$(CHECK_TEST_ERROR)
> +webapprt-test-chrome:
> +	$(RUN_MOCHITEST) --webapprt-chrome --appname $(webapprt_stub_path) --browser-arg -test-mode
> +	$(CHECK_TEST_ERROR)
> +endif

Do something like this instead so that the targets are defined only once:

> ifeq ($(OS_ARCH),Darwin)
> webapprt_stub_path = $(TARGET_DIST)/$(MOZ_MACBUNDLE_NAME)/Contents/MacOS/webapprt-stub$(BIN_SUFFIX)
> endif
> ifeq ($(OS_ARCH),WINNT)
> webapprt_stub_path = $(TARGET_DIST)/bin/webapprt-stub$(BIN_SUFFIX)
> endif
> 
> ifdef webapprt_stub_path
> webapprt-test-content:
> 	$(RUN_MOCHITEST) --webapprt-content --appname $(webapprt_stub_path)
> 	$(CHECK_TEST_ERROR)
> webapprt-test-chrome:
> 	$(RUN_MOCHITEST) --webapprt-chrome --appname $(webapprt_stub_path) --browser-arg -test-mode
> 	$(CHECK_TEST_ERROR)
> endif

::: webapprt/CommandLineHandler.js
@@ +31,5 @@
>  
>      if (!inTestMode) {
>        startUp();
>      } else {
> +      DOMApplicationRegistry.allAppsLaunchable = true;

Thanks for fixing this.

::: webapprt/win/webapprt.cpp
@@ +442,5 @@
>    }
>  
> +  // Check if the runtime was executed with the "-profile" argument
> +  bool profileAbsolute = false;
> +  for (int i=1; i<argc; i++)

Nit: spaces around = and <

@@ +444,5 @@
> +  // Check if the runtime was executed with the "-profile" argument
> +  bool profileAbsolute = false;
> +  for (int i=1; i<argc; i++)
> +    if (!strcmp(argv[i], "-profile")) {
> +      strcpy(profile, argv[i+1]);

Use strncpy or snprintf so you don't overrun the buffer.  (Also, nit: spaces around +.)

Actually I don't think you need to fill profile or set webShellAppData->profile in this case.  XRE looks for a profile in argv before looking at nsXREAppData->profile.  I'm pretty sure that's true, and it's how webapprt.mm works, but could you check me on that?

Although I don't know why webShellAppData->name is also set to profile.  Do you run into problems if name isn't set?  If that's the case, I'm OK with filling profile.

@@ +449,5 @@
> +      profileAbsolute = true;
> +    }
> +
> +  // First attempt at loading Firefox binaries:
> +  //   Check if the webapprt is in the same directory as the Firefox binary

Expand this comment to briefly mention that this is the case during WebappRT chrome and content tests.

Comment 9

[Marco Castelluccio [:marco]]

(In reply to Drew Willcoxon :adw from comment #8)
> Actually I don't think you need to fill profile or set
> webShellAppData->profile in this case.  XRE looks for a profile in argv
> before looking at nsXREAppData->profile.  I'm pretty sure that's true, and
> it's how webapprt.mm works, but could you check me on that?

Yes, I've already checked. I've leaved the code as is to not complicate it (otherwise I'd need to pass the profileAbsolute bool to AttemptGRELoadAndLaunch, or make it a global variable).

> Although I don't know why webShellAppData->name is also set to profile.  Do
> you run into problems if name isn't set?  If that's the case, I'm OK with
> filling profile.

If the name isn't set, you can't run multiple applications (on Win and Linux). I've assumed we will do tests with multiple applications running.

Thank you for the fast review!

Comment 10

[Myk Melez [:myk] [@mykmelez]]

When I build with the patch on Windows 7 and then try to run chrome or content tests (path/to/pymake webapprt-test-chrome), I get a Profile Missing dialog with a message like:

  Your c:\users\myk\appdata\local\temp\tmpf3zq_o/ profile cannot be loaded. It may be missing or inaccessible.

Comment 11

[Marco Castelluccio [:marco]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #10)
> When I build with the patch on Windows 7 and then try to run chrome or
> content tests (path/to/pymake webapprt-test-chrome), I get a Profile Missing
> dialog with a message like:
> 
>   Your c:\users\myk\appdata\local\temp\tmpf3zq_o/ profile cannot be loaded.
> It may be missing or inaccessible.

Now I get the same error, I'll investigate and update the patch today. I wonder why this error didn't show up before.

Comment 12

[Marco Castelluccio [:marco]]

Attachment: Patch v2

Ok, looks like I mustn't set nsXREAppData->profile and name.

Comment 13

[Drew Willcoxon :adw]

Comment on attachment 641638 [details] [diff] [review]
Patch v2

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

I'm a little concerned about the "profile cannot be loaded" problem because it sounds like we don't understand it.  (I don't.)  Why didn't it show up in your initial testing?  What changed to cause it?  Why does not setting nsXREAppData->name and ->profile fix it?

But if you've tested the latest patch on a recent tree and it works, then it works, and if problems happen later we can fix them then.

::: webapprt/win/webapprt.cpp
@@ +40,5 @@
>    wchar_t curExePath[MAXPATHLEN];
>    wchar_t backupFilePath[MAXPATHLEN];
>    wchar_t iconPath[MAXPATHLEN];
>    char profile[MAXPATHLEN];
> +  bool profileAbsolute = false;

profileAbsolute isn't a great name since it's not clear what it means.  I think it means profilePathIsAbsolute or profileNameIsAbsolutePath, right?  It also doesn't hint at the fact that if it's true, then the `profile` string doesn't matter.  So I think a better name would be something like profileOverridden, profileGiven, profileUserDefined, or whatever you can think of.  Up to you.

@@ +446,5 @@
>  
> +  // Check if the runtime was executed with the "-profile" argument
> +  for (int i = 1; i < argc; i++) {
> +    if (!strcmp(argv[i], "-profile")) {
> +      profileAbsolute = true;

Nit: you could break here.

Comment 14

[Marco Castelluccio [:marco]]

Attachment: Patch v3

Comment 15

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 642104 [details] [diff] [review]
Patch v3

Carrying forward Drew's review, as this bug just fixes his nits (with a bit of input from me over IRC on the naming).

https://hg.mozilla.org/integration/mozilla-inbound/rev/d573e0604c26

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

tracking for 16 in case this doesn't make it into central before merge day on monday.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d573e0604c26