860683 - Route browser output through metrotestharness stdout

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Tests, defect)

Description

[Jim Mathies [:jimm]]

The patch in bug 855407 partially fixed this by routing browser stdout over to the metrotestharness console. However, when automation runs tests, the mozilla test harness doesn't cat the console, it creates and sets a pipe as stdout for the subprocess it launches using python's subproccess.POpen. So browser stdout can't just dump to the same console, it has to dump to metrotestharness stdout. According to the MSDN docs on DuplicateHandle, we can't share stdout handles across processes, so we'll have to capture browser output some other way and dump it in metrotestharness.

Comment 1

[Jim Mathies [:jimm]]

Attachment: wip

I don't think the console id passing is useful anymore since we can rely on the named pipe instead, so this needs some cleanup. Pushing this to try for a build armen can test with.

Comment 2

[Jim Mathies [:jimm]]

Attachment: browser v.1

Comment 3

[Jim Mathies [:jimm]]

Attachment: winrt logging

Converting this over to ascii output so it shows up right on the other side of the pipe.

Comment 4

[Jim Mathies [:jimm]]

https://tbpl.mozilla.org/?tree=Try&rev=3ffd00db6bd3

Armen, would you please take this build for a spin to see if the browser output shows up where it needs to?

Comment 5

[Brian R. Bondy [:bbondy]]

Comment on attachment 736239 [details] [diff] [review]
browser v.1

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

Have you tried setting the hStdOutput member of STARTUPINFO?
http://msdn.microsoft.com/en-ca/library/windows/desktop/ms686331%28v=vs.85%29.aspx

::: browser/metro/shell/testing/metrotestharness.cpp
@@ +28,5 @@
>  static const WCHAR* kDefaultMetroBrowserIDPathKey = L"FirefoxURL";
>  static const WCHAR* kDemoMetroBrowserIDPathKey = L"Mozilla.Firefox.URL";
>  
> +// Logging pipe handle
> +HANDLE gTestOutputPipe = NULL;

nit: gTestOutputPipe = INVALID_HANDLE_VALUE;
Since this is what CreateNamedPipe returns on failure.

@@ +166,5 @@
>  {
> +  SECURITY_ATTRIBUTES saAttr;
> +  saAttr.nLength = sizeof(SECURITY_ATTRIBUTES); 
> +  saAttr.bInheritHandle = TRUE; 
> +  saAttr.lpSecurityDescriptor = NULL; 

nit: trailing whtespace

::: widget/windows/winrt/MetroUtils.cpp
@@ +83,5 @@
>    OutputDebugStringW(szDebugString);
>    OutputDebugStringW(L"\n");
>  
>    // desktop console
> +  //wprintf(L"%s\n", szDebugString);

why is this commented out?

Comment 6

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #5)
> >    // desktop console
> > +  //wprintf(L"%s\n", szDebugString);
> 
> why is this commented out?

It shows up as "?????????????????????????????????????" output on the other side of the pipe because it's not converted to ascii. Hence the follow up patch to convert winrt logging over.

Comment 7

[Armen [:armenzg]]

Attachment: metro log

I think it works. How does the log look like?

Steps followed:
cd C:\slave\test
rmdir /s /q build scripts
C:\mozilla-build\hg\hg.exe clone http://hg.mozilla.org/users/armenzg_mozilla.com/mozharness scripts
c:/mozilla-build/python27/python -u scripts/scripts/desktop_unittest.py --cfg unittests/win_unittest.py --mochitest-suite metro-immersive --download-symbols ondemand --installer-url http://people.mozilla.com/~armenzg/metro_try_build/firefox-23.0a1.en-US.win32.zip --test-url http://people.mozilla.com/~armenzg/metro_try_build/firefox-23.0a1.en-US.win32.tests.zip --no-read-buildbot-config

Comment 8

[Jim Mathies [:jimm]]

(In reply to Armen Zambrano G. [:armenzg] from comment #7)
> Created attachment 736438 [details] [diff] [review]
> metro log
> 
> I think it works. How does the log look like?
> 
> Steps followed:
> cd C:\slave\test
> rmdir /s /q build scripts
> C:\mozilla-build\hg\hg.exe clone
> http://hg.mozilla.org/users/armenzg_mozilla.com/mozharness scripts
> c:/mozilla-build/python27/python -u scripts/scripts/desktop_unittest.py
> --cfg unittests/win_unittest.py --mochitest-suite metro-immersive
> --download-symbols ondemand --installer-url
> http://people.mozilla.com/~armenzg/metro_try_build/firefox-23.0a1.en-US.
> win32.zip --test-url
> http://people.mozilla.com/~armenzg/metro_try_build/firefox-23.0a1.en-US.
> win32.tests.zip --no-read-buildbot-config

Your post has line wrap that looks a little funny, did you pull it out of a console window? If so, not an issue. Looks like it's working!

Comment 9

[Jim Mathies [:jimm]]

Attachment: browser v.2

Comment 10

[Jim Mathies [:jimm]]

Comment on attachment 736458 [details] [diff] [review]
browser v.2

woops, commingling patches here.

Comment 11

[Jim Mathies [:jimm]]

Attachment: browser v.2

Comment 12

[Jim Mathies [:jimm]]

Attachment: convert winrt log output to ascii

Comment 13

[Brian R. Bondy [:bbondy]]

Comment on attachment 736460 [details] [diff] [review]
convert winrt log output to ascii

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

::: widget/windows/winrt/MetroContracts.cpp
@@ +58,5 @@
>    if (WindowsIsStringEmpty(data.Get()))
>      return;
>  
>    unsigned int length;
> +  LogW(L"SearchActivated text=", data.GetRawBuffer(&length));

Is this missing a formatting specifier?

@@ +135,5 @@
>    for (int i = 0; i < argc; ++i) {
>      NS_ConvertUTF16toUTF8 arg(argv[i]);
>      argvUTF8[i] = new char[arg.Length() + 1];
>      strcpy(argvUTF8[i], const_cast<char *>(arg.BeginReading()));
> +    LogW(L"Launch arg[%d]: '%s'", i, argv[i]);

argvUTF8[i]?

::: widget/windows/winrt/MetroWidget.cpp
@@ +104,4 @@
>              inputs[i].ki.wVk,
>              inputs[i].ki.dwFlags & KEYEVENTF_KEYUP
>              ? L"UP"
>              : L"DOWN");

? "UP"
: "DOWN"

::: widget/windows/winrt/UIABridge.cpp
@@ +143,5 @@
>      return;
>    }
>    nsString str;
>    aChild->GetName(str);
> +  Log("name: %s", str.BeginReading());

%ls or LogW(L ?

@@ +148,2 @@
>    aChild->GetDescription(str);
> +  Log("description: %s", str.BeginReading());

%ls or LogW(L ?

Comment 14

[Jim Mathies [:jimm]]

Attachment: browser v.3

Comment 15

[Jim Mathies [:jimm]]

Comment on attachment 736494 [details] [diff] [review]
browser v.3

w/o the commented out wprintf.

Comment 16

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #13)
> Comment on attachment 736460 [details] [diff] [review]
> convert winrt log output to ascii
> 
> Review of attachment 736460 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/winrt/MetroContracts.cpp
> @@ +58,5 @@
> >    if (WindowsIsStringEmpty(data.Get()))
> >      return;
> >  
> >    unsigned int length;
> > +  LogW(L"SearchActivated text=", data.GetRawBuffer(&length));
> 
> Is this missing a formatting specifier?

heh, that's been in there for a while.

> @@ +135,5 @@
> >    for (int i = 0; i < argc; ++i) {
> >      NS_ConvertUTF16toUTF8 arg(argv[i]);
> >      argvUTF8[i] = new char[arg.Length() + 1];
> >      strcpy(argvUTF8[i], const_cast<char *>(arg.BeginReading()));
> > +    LogW(L"Launch arg[%d]: '%s'", i, argv[i]);
> 
> argvUTF8[i]?

I used LogW here, so this should be ok assuming argv was wide to begin with. Pretty sure it is, we call NS_ConvertUTF16toUTF8 on it.

> ? "UP"
> : "DOWN"

updated.

> ::: widget/windows/winrt/UIABridge.cpp
> @@ +143,5 @@
> >      return;
> >    }
> >    nsString str;
> >    aChild->GetName(str);
> > +  Log("name: %s", str.BeginReading());
> 
> %ls or LogW(L ?

nice catch. updated.

> 
> @@ +148,2 @@
> >    aChild->GetDescription(str);
> > +  Log("description: %s", str.BeginReading());
> 
> %ls or LogW(L ?

updated.

Comment 17

[Jim Mathies [:jimm]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/17d705ba2d9b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3880c0cfd288

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/17d705ba2d9b
https://hg.mozilla.org/mozilla-central/rev/3880c0cfd288