Closed Bug 771271 Opened 7 years ago Closed 7 years ago

Get mochitest-metro-chrome tests running in immersive mode

Categories

(Core :: General, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: bbondy, Assigned: jimm)

References

Details

(Whiteboard: [completed-elm][metro-it2])

Attachments

(2 files, 9 obsolete files)

We should find a way to invoke xpcshell tests, mochitests, ... inside the Metro browser.   We may need to invoke a shortcut so the CommandExecuteHandler forces a switch to Metro mode first.  

We will also need to find a way to register ourselves as the default browser from within the directory we will run the tests in. 

This bug is to track that work.
Summary: Find a way to run tests in Metro mode in Windows 8. → Find a way to run tests in Metro mode in Windows 8
Depends on: 759905
Automating setting the default browser is going to be hard. We can populate the proper registry values but I'm not sure how we would then set the default via Windows UI. Maybe we can do this through some sort of helper app or preconfigure the default on the slaves based on where tests will be located.

Launching and running existing tests will also have issues as most test suites require two windows, and tests often create additional windows when they run.

We might want to limit the scope of this to metro browser chrome tests since we get test coverage for platform through desktop builds. Additional tests for winrt widget backend issues can be added to metro browser tests when we find an issue that requires a specific test.
will file a follow up on talos tests.
Summary: Find a way to run tests in Metro mode in Windows 8 → Get mochitest-metro-chrome tests running in immersive mode
Getting launched in immersive mode is tricky. IApplicationActivationManager's ActivateApplication doesn't allow deb launch into immerisve mode so it's not an option here. Seems like the only way to do this may require launching via a shortcut that has the PKEY_AppUserModel_IsDualMode property, maybe with a command line flag that tells the ceh we want to run in the immersive environment.
Update on this - IApplicationActivationManager's ActivateApplication succeeds in launching the metro browser and the browser is successful at creating a CoreWindow and activating it. Strange thing is though the metro interface seems unaware this app is running, there's no entry in the thumbnail view for the browser at all, and the metro environment doesn't open after the call to ActivateApplication. I wonder if this might be a bug in release preview.
Attached patch wip (obsolete) — Splinter Review
Assignee: nobody → jmathies
Note ActivateApplication doesn't work for 'Microsoft.InternetExplorer.Default' either.
Attached patch wip v2 (obsolete) — Splinter Review
Attachment #642385 - Attachment is obsolete: true
Attached patch wip v3 (obsolete) — Splinter Review
This is working, just needs some cleanup and reviews.
Attachment #642525 - Attachment is obsolete: true
Attached patch wip v3 (obsolete) — Splinter Review
correct patch file.
Attachment #643702 - Attachment is obsolete: true
Attached patch browserapp patch (obsolete) — Splinter Review
This will need a /browser peer review for the elm merge.
Attachment #643703 - Attachment is obsolete: true
Attached patch ceh changes (obsolete) — Splinter Review
Attached patch metro test harness patch (obsolete) — Splinter Review
This is the metro test harness parts. The way this works:

1) adds a new command line option ‘--metro-immersive’ to runtests.py. What this does is swap firefox.exe with metrotestharness.exe for launch.
2) Since we can’t pass command line args to firefox.exe when it’s activated through COM so metrotestharness writes commands to a tests.ini file. In nsBrowserApp, we check for this file and read in the command line args we need for testing.
3) metrotestharness activates the default metro browser. Note, we have yet to find a way to programmatically set the default metro browser, so this has to be set before tests are run.
4) metrotestharness waits for the browser to exit then calls a shell command to flip back to the desktop.
Attachment #643828 - Flags: review?(benjamin)
Attached patch widget bits (obsolete) — Splinter Review
Comment on attachment 643828 [details] [diff] [review]
metro test harness patch

sorry, didn't want reviews yet, just feedback on the general way this works. I can get bbondy to review the actual code.
Attachment #643828 - Flags: review?(benjamin) → feedback?(benjamin)
Blocks: 769281
Blocks: 771293
Depends on: 768464
Comment on attachment 643828 [details] [diff] [review]
metro test harness patch

I'm going away on PTO, try khuey for this?
Attachment #643828 - Flags: feedback?(benjamin)
Comment on attachment 643828 [details] [diff] [review]
metro test harness patch

Kyle, this is a test harness we use to get tests running in the win8 metro environment. I'd like to land this on elm for the time being so we can run tests locally. 

There are a couple limitations with the current code - the browser you test must be pre-configured as the default metro browser since we haven't found a way to set this config up through code. We don't anticipate this to be a major issue for rel-eng when they start work on automating this. Also this work is blocked by bug 768464 which will handle flipping from metro back to the desktop when tests complete.
Attachment #643828 - Flags: feedback?(khuey)
One of the issues with running tests in imersive mode is the lack of the ability to pass command line args to the browser. Through these patches what we do basically is write out a ini file with this information. The test harness does the writing and the reading occurs in browserapp patch.
Blocks: metro-talos
Comment on attachment 643828 [details] [diff] [review]
metro test harness patch

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

I'm not a test harness peer.
Attachment #643828 - Flags: feedback?(khuey) → feedback?(ted.mielczarek)
No longer blocks: 771293
There might be applicable info on immersive mode automated testing here:
http://blogs.msdn.com/b/windowsappdev/archive/2012/09/04/automating-the-testing-of-windows-8-apps.aspx
Attachment #643828 - Flags: feedback?(ted.mielczarek)
I'm going to update these and land them on elm since we need to get test coverage going.

We can do reviews when we merge to mc.
Comment on attachment 643827 [details] [diff] [review]
ceh changes

These changes already landed on elm.
Attachment #643827 - Attachment is obsolete: true
Whiteboard: completed-elm
Depends on: 803671
Blocks: metro-misc
Attachment #643826 - Attachment is obsolete: true
Attachment #643828 - Attachment is obsolete: true
Attachment #643829 - Attachment is obsolete: true
Attached patch test harness v.1Splinter Review
This adds a new test option for chrome tests that run in win8's immersive mode.
Attachment #678319 - Flags: review?(jmaher)
Attached patch browser app v.1 (obsolete) — Splinter Review
These changes pick up a new test related ini file created when we run tests in immersive mode. Command line params aren't possible here since the os launches the browser in response to a request we make in the metro test harness.

The metro test harness can be viewed here - 

http://mxr.mozilla.org/projects-central/source/elm/browser/metro/shell/testing/metrotestharness.cpp
Attachment #678323 - Flags: review?(gavin.sharp)
Comment on attachment 678319 [details] [diff] [review]
test harness v.1

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

Does this require --metro-immersive && --browserChrome?  If so, I believe we need to fix runtests.py, if not this looks good.

r- for the unenforced browser-chrome requirement, please correct me if I am wrong.

::: browser/build.mk
@@ +88,5 @@
>  .PHONY: mochitest-browser-chrome
> +
> +mochitest-metro-chrome:
> +	$(RUN_MOCHITEST) --metro-immersive --browser-chrome
> +	$(CHECK_TEST_ERROR)

can we get a mach command for this?
Attachment #678319 - Flags: review?(jmaher) → review-
(In reply to Joel Maher (:jmaher) from comment #24)
> Does this require --metro-immersive && --browserChrome?  If so, I believe we
> need to fix runtests.py, if not this looks good.

If I understand the question correctly:  --metro-immersive and --browser-chrome control different things and can be used separately or together.

--metro-immersive --browser-chrome runs the Metro browser-chrome tests.

--browser-chrome alone runs the desktop browser-chrome tests.

--metro-immersive alone runs plain mochitests in the Metro browser (though I don't know whether this is fully working yet).
(In reply to Joel Maher (:jmaher) from comment #24)
> can we get a mach command for this?

Is there a wiki page or other documentation on doing this? I haven't messed with mach yet.
I couldn't find one after a few google searches.  We could file a followup bug for the mach integration.
Comment on attachment 678323 [details] [diff] [review]
browser app v.1

I'm not the best person to review this - perhaps glandium or Brian Bondy can take a look?
Attachment #678323 - Flags: review?(gavin.sharp)
(In reply to Matt Brubeck (:mbrubeck) from comment #25)
> (In reply to Joel Maher (:jmaher) from comment #24)
> > Does this require --metro-immersive && --browserChrome?  If so, I believe we
> > need to fix runtests.py, if not this looks good.
> 
> If I understand the question correctly:  --metro-immersive and
> --browser-chrome control different things and can be used separately or
> together.
> 
> --metro-immersive --browser-chrome runs the Metro browser-chrome tests.
> 
> --browser-chrome alone runs the desktop browser-chrome tests.
> 
> --metro-immersive alone runs plain mochitests in the Metro browser (though I
> don't know whether this is fully working yet).

Did this answer your question Joel? If so and the answer was sufficient, can you switch the r- to r+? If not, please let me know what needs to change.
Attachment #678323 - Flags: review?(netzen)
Comment on attachment 678323 [details] [diff] [review]
browser app v.1

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

::: browser/app/nsBrowserApp.cpp
@@ +131,4 @@
>  {
>    nsCOMPtr<nsIFile> appini;
>    nsresult rv;
> +  uint32_t mainFlags = 0;

Could we just move this down to avoid unused variable warnings on other platforms?

@@ +183,5 @@
> +  if (argc > 1) {
> +    // This command-line flag is passed to our executable when it is to be
> +    // launched in metro mode (i.e. our EXE is registered as the default
> +    // browser and the user has tapped our EXE's tile)
> +    if (IsArg(argv[1], "ServerName:DefaultBrowserServer")) {

I think we are, but are you sure this will always be the second argv?

@@ +272,5 @@
> +    return 255;
> +  }
> +  appDirectory.forget(&appData->directory);
> +
> +#ifdef XP_WIN

Is there any value in running the Metro front end tests on other platforms?

@@ +278,5 @@
> +    char testFile[MAXPATHLEN+1];
> +
> +    memset(testFile, 0, sizeof(testFile));
> +
> +    strncpy(testFile, aWorkingPath, MAXPATHLEN);

Why not just call GetCurrentDirectoryW and then you can work with unicode paths and you know it'll be set correctly?

@@ +283,5 @@
> +    strncat(testFile, XPCOM_FILE_PATH_SEPARATOR, MAXPATHLEN);
> +    strncat(testFile, "tests.ini", MAXPATHLEN);
> +
> +    // Check for a metro test harness command line args file
> +    HANDLE hTestFile = CreateFileA(testFile, GENERIC_READ,

nit: You can use nsAutoHandle from nsWindowsHelpers.h here

@@ +292,5 @@
> +      // Typical test harness command line args string is around 100 bytes.
> +      char buffer[1024];
> +      memset(buffer, 0, sizeof(buffer));
> +      DWORD bytesRead = 0;
> +      if (!ReadFile(hTestFile, (VOID*)buffer, sizeof(buffer)-1,

If you think this will ever get > 1k then pls put it in a loop. But I think that's fine as is.

@@ +306,5 @@
> +      int newArgc = 1;
> +
> +      memset(newArgv, 0, sizeof(newArgv));
> +
> +      char* ptr = buffer;

Could you add a comment on why you are building a new array of args here? So we're replacing the args with the ones from the file right? How will that differ? I'm assuming you set these from the CEH is that right? If so what does that set in particular? Couldn't we just add one here?

@@ +320,5 @@
> +          continue;
> +        }
> +        ptr++;
> +      }
> +      newArgc--;

Should we remove that file that was previously created, and opened in this file, here?

@@ +359,5 @@
>  int main(int argc, char* argv[])
>  {
>    PRTime start = _PR_Now();
>    char exePath[MAXPATHLEN];
> +  char workingPath[MAXPATHLEN];

I don't think this is used.

@@ +379,5 @@
> +  // Save the working dir for later use
> +  memset(workingPath, 0, sizeof(workingPath));
> +  strncpy(workingPath, exePath, (lastSlash - exePath));
> +
> +  // concat the dll name

I don't think this is used.
Ignore the comment about if we want this Windows only. I said that before I knew this was related to the CEH.
Comment on attachment 678319 [details] [diff] [review]
test harness v.1

thanks for clarifying how this runs.  Do we need a mochitest-metro option for mochitests?
Attachment #678319 - Flags: review- → review+
(In reply to Joel Maher (:jmaher) from comment #32)
> Comment on attachment 678319 [details] [diff] [review]
> test harness v.1
> 
> thanks for clarifying how this runs.  Do we need a mochitest-metro option
> for mochitests?

Existing platform mochitest tests should run in desktop mode. A lot of tests would fail in metro due to windowing constraints. So I don't think we'll need support for running platform tests in metro.
> @@ +278,5 @@
> > +    char testFile[MAXPATHLEN+1];
> > +
> > +    memset(testFile, 0, sizeof(testFile));
> > +
> > +    strncpy(testFile, aWorkingPath, MAXPATHLEN);
> 
> Why not just call GetCurrentDirectoryW and then you can work with unicode
> paths and you know it'll be set correctly?

I suppose but I already have aWorkingPath available. I'm passing in aWorkingPath which is ascii and is needed in setting up the path info in appData, which is cross-platform code. aWorkingPath is derived from exePath in main(), which is passed in on the command line and processed there.

> @@ +359,5 @@
> >  int main(int argc, char* argv[])
> >  {
> >    PRTime start = _PR_Now();
> >    char exePath[MAXPATHLEN];
> > +  char workingPath[MAXPATHLEN];
> 
> I don't think this is used.
> 
> @@ +379,5 @@
> > +  // Save the working dir for later use
> > +  memset(workingPath, 0, sizeof(workingPath));
> > +  strncpy(workingPath, exePath, (lastSlash - exePath));
> > +
> > +  // concat the dll name
> 
> I don't think this is used.

This does really, we need an ascii version of the working dir for appData. exePath is pulled from the command line params and parsed via mozilla::BinaryPath::Get(). exePath is used in a couple ways for xpcom glue startup. aWorkingPath gets passed in do_main().
(In reply to Brian R. Bondy [:bbondy] from comment #30)
> ::: browser/app/nsBrowserApp.cpp
> @@ +131,4 @@
> >  {
> >    nsCOMPtr<nsIFile> appini;
> >    nsresult rv;
> > +  uint32_t mainFlags = 0;
> 
> Could we just move this down to avoid unused variable warnings on other
> platforms?

updated.

> @@ +183,5 @@
> > +  if (argc > 1) {
> > +    // This command-line flag is passed to our executable when it is to be
> > +    // launched in metro mode (i.e. our EXE is registered as the default
> > +    // browser and the user has tapped our EXE's tile)
> > +    if (IsArg(argv[1], "ServerName:DefaultBrowserServer")) {
> 
> I think we are, but are you sure this will always be the second argv?

From all of our testing Windows always places it in the first position.

> @@ +283,5 @@
> > +    strncat(testFile, XPCOM_FILE_PATH_SEPARATOR, MAXPATHLEN);
> > +    strncat(testFile, "tests.ini", MAXPATHLEN);
> > +
> > +    // Check for a metro test harness command line args file
> > +    HANDLE hTestFile = CreateFileA(testFile, GENERIC_READ,
> 
> nit: You can use nsAutoHandle from nsWindowsHelpers.h here

Updated.

> 
> @@ +306,5 @@
> > +      int newArgc = 1;
> > +
> > +      memset(newArgv, 0, sizeof(newArgv));
> > +
> > +      char* ptr = buffer;
> 
> Could you add a comment on why you are building a new array of args here? So
> we're replacing the args with the ones from the file right? How will that
> differ? I'm assuming you set these from the CEH is that right? If so what
> does that set in particular? Couldn't we just add one here?

Added a longer comment.

If the ini file is present, there will only two incoming args from Windows, the exe path and 'ServerName:DefaultBrowserServer'. The ini file is written out by the metrotestharness.exe that gets invoked by our tests. So there's no point in passing those two args down with the args we generate.

> 
> @@ +320,5 @@
> > +          continue;
> > +        }
> > +        ptr++;
> > +      }
> > +      newArgc--;
> 
> Should we remove that file that was previously created, and opened in this
> file, here?

Not sure I understand your question. tests.ini is created and deleted by metrotestharness.exe.

http://mxr.mozilla.org/projects-central/source/elm/browser/metro/shell/testing/metrotestharness.cpp#134
> > Should we remove that file that was previously created, and opened in this
> > file, here?


> Not sure I understand your question. tests.ini is created and 
> deleted by metrotestharness.exe.

I didn't have the code for what writes test.ini in front of me so I just assumed it was being written out by the CEH.  So ya that comment can be ignored.
Attached patch browser app v.2Splinter Review
Updated to comments.
Attachment #678323 - Attachment is obsolete: true
Attachment #678323 - Flags: review?(netzen)
Attachment #679359 - Flags: review?(netzen)
Comment on attachment 679359 [details] [diff] [review]
browser app v.2

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

Looks good. I don't think the passed char* path is utf-8 so I'm not sure if this will work when tests are run from a directory with unicode chars, but I don't think we should block on that if that's even a problem.

Thanks for quoting this file:
http://mxr.mozilla.org/projects-central/source/elm/browser/metro/shell/testing/metrotestharness.cpp#134
It helped me understand more :)
Attachment #679359 - Flags: review?(netzen) → review+
No longer depends on: 768464
No longer depends on: 803671
Duplicate of this bug: 803671
(In reply to Jim Mathies [:jimm] from comment #37)
> Created attachment 679359 [details] [diff] [review]
> browser app v.2
> 
> Updated to comments.

Looks like this is going to have to wait, there's code in here for other platforms specific to the metro-build bug.
Depends on: metro-build
No longer depends on: metro-build
Blocks: elm-merge
No longer blocks: metro-misc
Whiteboard: completed-elm → [completed-elm][metro-it2]
Depends on: 825034
Blocks: 828691
No longer blocks: elm-merge
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
No longer blocks: metro-talos
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.