Closed
Bug 771271
Opened 12 years ago
Closed 11 years ago
Get mochitest-metro-chrome tests running in immersive mode
Categories
(Core :: General, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: bbondy, Assigned: jimm)
References
Details
(Whiteboard: [completed-elm][metro-it2])
Attachments
(2 files, 9 obsolete files)
5.71 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
8.99 KB,
patch
|
bbondy
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•12 years ago
|
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
Assignee | ||
Comment 1•12 years ago
|
||
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.
Assignee | ||
Comment 2•12 years ago
|
||
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
Assignee | ||
Comment 3•12 years ago
|
||
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.
Assignee | ||
Comment 4•12 years ago
|
||
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.
Assignee | ||
Comment 5•12 years ago
|
||
Assignee: nobody → jmathies
Assignee | ||
Comment 6•12 years ago
|
||
Note ActivateApplication doesn't work for 'Microsoft.InternetExplorer.Default' either.
Assignee | ||
Comment 7•12 years ago
|
||
Attachment #642385 -
Attachment is obsolete: true
Updated•12 years ago
|
Blocks: metro-testing
Assignee | ||
Comment 8•12 years ago
|
||
This is working, just needs some cleanup and reviews.
Attachment #642525 -
Attachment is obsolete: true
Assignee | ||
Comment 9•12 years ago
|
||
correct patch file.
Attachment #643702 -
Attachment is obsolete: true
Assignee | ||
Comment 10•12 years ago
|
||
This will need a /browser peer review for the elm merge.
Attachment #643703 -
Attachment is obsolete: true
Assignee | ||
Comment 11•12 years ago
|
||
Assignee | ||
Comment 12•12 years ago
|
||
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)
Assignee | ||
Comment 13•12 years ago
|
||
Assignee | ||
Comment 14•12 years ago
|
||
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)
Comment 15•12 years ago
|
||
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)
Assignee | ||
Comment 16•12 years ago
|
||
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)
Assignee | ||
Comment 17•12 years ago
|
||
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.
Assignee | ||
Updated•12 years ago
|
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)
Reporter | ||
Comment 19•12 years ago
|
||
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
Assignee | ||
Updated•12 years ago
|
Attachment #643828 -
Flags: feedback?(ted.mielczarek)
Assignee | ||
Comment 20•12 years ago
|
||
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.
Assignee | ||
Comment 21•12 years ago
|
||
Comment on attachment 643827 [details] [diff] [review] ceh changes These changes already landed on elm.
Attachment #643827 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Whiteboard: completed-elm
Assignee | ||
Updated•12 years ago
|
Blocks: metro-misc
Assignee | ||
Updated•12 years ago
|
Attachment #643826 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #643828 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #643829 -
Attachment is obsolete: true
Assignee | ||
Comment 22•12 years ago
|
||
This adds a new test option for chrome tests that run in win8's immersive mode.
Attachment #678319 -
Flags: review?(jmaher)
Assignee | ||
Comment 23•12 years ago
|
||
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 24•12 years ago
|
||
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-
Comment 25•12 years ago
|
||
(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).
Assignee | ||
Comment 26•12 years ago
|
||
(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.
Comment 27•12 years ago
|
||
I couldn't find one after a few google searches. We could file a followup bug for the mach integration.
Comment 28•12 years ago
|
||
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)
Assignee | ||
Comment 29•12 years ago
|
||
(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.
Assignee | ||
Updated•12 years ago
|
Attachment #678323 -
Flags: review?(netzen)
Reporter | ||
Comment 30•12 years ago
|
||
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.
Reporter | ||
Comment 31•12 years ago
|
||
Ignore the comment about if we want this Windows only. I said that before I knew this was related to the CEH.
Comment 32•12 years ago
|
||
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+
Assignee | ||
Comment 33•12 years ago
|
||
(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.
Assignee | ||
Comment 34•12 years ago
|
||
> @@ +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().
Assignee | ||
Comment 35•12 years ago
|
||
(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
Reporter | ||
Comment 36•12 years ago
|
||
> > 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.
Assignee | ||
Comment 37•12 years ago
|
||
Updated to comments.
Attachment #678323 -
Attachment is obsolete: true
Attachment #678323 -
Flags: review?(netzen)
Attachment #679359 -
Flags: review?(netzen)
Reporter | ||
Comment 38•12 years ago
|
||
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+
Assignee | ||
Comment 40•12 years ago
|
||
(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
Assignee | ||
Updated•12 years ago
|
No longer depends on: metro-build
Assignee | ||
Updated•12 years ago
|
No longer blocks: metro-misc
Assignee | ||
Updated•11 years ago
|
Whiteboard: completed-elm → [completed-elm][metro-it2]
Assignee | ||
Updated•11 years ago
|
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 41•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e86fce88995e
Assignee | ||
Updated•11 years ago
|
No longer blocks: metro-talos
Updated•10 years ago
|
OS: Windows 8 Metro → Windows 8.1
You need to log in
before you can comment on or make changes to this bug.
Description
•