Bug 56729 (nrwt-wk2)

Summary: Switch webkit2 bot to NRWT
Product: WebKit Reporter: Maciej Stachowiak <mjs>
Component: Tools / TestsAssignee: Eric Seidel (no email) <eric>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben, dpranke, eric, laszlo.gombos, lforschler, simon.fraser, webkit-bug-importer, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 63439, 63501, 63673, 63683, 63780, 64056, 67530, 70371, 70435    
Bug Blocks: 34984, 66965    
Attachments:
Description Flags
Patch none

Description Maciej Stachowiak 2011-03-20 21:15:00 PDT
new-run-webkit-tests lacks old-run-webkit-tests' support for WebKit2. This consists of the --2 option which uses the WebKitTestRunner tool, and support for detecting WebProcess crashes.
Comment 1 Eric Seidel (no email) 2011-06-26 23:28:01 PDT
OK.  I'm working on NRWT again.  It's unclear how much work it will be to add this support.
Comment 2 Eric Seidel (no email) 2011-06-27 00:01:09 PDT
Adding support for using WebKitTestRunnner instead of DumpRenderTree seems trivial.
Comment 3 Eric Seidel (no email) 2011-06-27 00:04:24 PDT
Looks like this should be as simple as teaching the port how to return driver_name() of "WebKitTestRunner" when passed the --2 option, as well as teaching WebKitDriver._read_block how to understand #CRASHED and #CRASHED - WebProcess
Comment 4 Eric Seidel (no email) 2011-06-27 01:56:46 PDT
Did half of what is needed in bug 63439.  The harder part of actually making us talk to WebKitTestRunner (and supporting the new #CRASHED stuff) is left.
Comment 5 Adam Roben (:aroben) 2011-06-27 05:40:13 PDT
(In reply to comment #3)
> Looks like this should be as simple as teaching the port how to return driver_name() of "WebKitTestRunner" when passed the --2 option, as well as teaching WebKitDriver._read_block how to understand #CRASHED and #CRASHED - WebProcess

FYI, Apple's Windows port of DRT also prints #CRASHED in some cases, so it would be useful for #CRASHED to be understood even when using DRT.
Comment 6 Eric Seidel (no email) 2011-06-27 20:41:28 PDT
new-run-webkit-tests -2 "just works" after bug 63501.  I've not yet implemented the #CRASHED handling, but I'm not actually sure it's necessary.  We're still correctly detecting crashes, etc.

Does WebKitTestRunner continue running after a WebProcess crash, or does it itself exit?
Comment 7 Eric Seidel (no email) 2011-06-27 20:42:08 PDT
(My previous comment was not to imply that I'm in any way against the #CRASHED handling, just stating that new-run-webkit-tests -2 seems to work even though we haven't added it yet.)
Comment 8 Eric Seidel (no email) 2011-06-27 20:59:06 PDT
After the patch on bug 63501 lands, we still have some 20 unexpected failures with -2.  We can use this bug to track such.
Comment 9 Maciej Stachowiak 2011-06-28 00:36:52 PDT
(In reply to comment #6)
> new-run-webkit-tests -2 "just works" after bug 63501.  I've not yet implemented the #CRASHED handling, but I'm not actually sure it's necessary.  We're still correctly detecting crashes, etc.
> 
> Does WebKitTestRunner continue running after a WebProcess crash, or does it itself exit?

Yes, WebKitTestRunner continues after a WebProcess crash (unless the driver script tells it otherwise). That's kind of the point of the WebProcess, to not crash its client when it crashes.

Also, if WKTR made itself crash when WebProcess crashes, then results would show a backtrace of the wrong process.
Comment 10 Maciej Stachowiak 2011-06-28 00:38:22 PDT
(In reply to comment #7)
> (My previous comment was not to imply that I'm in any way against the #CRASHED handling, just stating that new-run-webkit-tests -2 seems to work even though we haven't added it yet.)

It should work without it, you just need to detect WebProcess crashes and hangs if you don't want the test session to hang indefinitely when it happens. Making the WebProcess crash or hang deliberately (using kill -SEGV to crash or kill -STOP to hang) should be a reasonable way to test it. It could be a separate bug.
Comment 11 Radar WebKit Bug Importer 2011-10-03 11:26:01 PDT
<rdar://problem/10224918>
Comment 12 Eric Seidel (no email) 2011-10-19 21:57:18 PDT
We're ready to pull the trigger.  Support is in.
Comment 13 Eric Seidel (no email) 2011-10-20 21:58:07 PDT
Created attachment 111902 [details]
Patch
Comment 14 Eric Seidel (no email) 2011-10-20 22:00:28 PDT
I've tested locally.  My machine sees 81 failures with WebKit2, in both ORWT and NRWT.  Which matches the 81 failures seen on the bot.
Comment 15 WebKit Review Bot 2011-10-20 23:48:16 PDT
Comment on attachment 111902 [details]
Patch

Clearing flags on attachment: 111902

Committed r98074: <http://trac.webkit.org/changeset/98074>
Comment 16 WebKit Review Bot 2011-10-20 23:48:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Simon Fraser (smfr) 2011-10-21 11:12:54 PDT
This seems to have caused lots of plugin tests to fail:
http://build.webkit.org/results/Lion%20Intel%20Release%20(Tests)/r98106%20(2090)/results.html
Comment 18 Eric Seidel (no email) 2011-10-21 11:15:21 PDT
(In reply to comment #17)
> This seems to have caused lots of plugin tests to fail:
> http://build.webkit.org/results/Lion%20Intel%20Release%20(Tests)/r98106%20(2090)/results.html

That builder doesn't use WK2 does it?  I"m not sure that's related to this specific change, but may be related to other changes which I made to support WK2?
Comment 19 Eric Seidel (no email) 2011-10-22 18:04:59 PDT
(In reply to comment #17)
> This seems to have caused lots of plugin tests to fail:
> http://build.webkit.org/results/Lion%20Intel%20Release%20(Tests)/r98106%20(2090)/results.html

Do you still believe this to be the case?
Comment 20 Simon Fraser (smfr) 2011-10-24 11:30:57 PDT
I think so. Maybe the test plugin isn't getting built?
Comment 21 Eric Seidel (no email) 2011-10-24 11:55:00 PDT
Behavior on the lion bot (non-wk2) should not have changed (at least not intentionally).

It's unclear which of the tests you cite from:
http://build.webkit.org/results/Lion%20Intel%20Release%20(Tests)/r98106%20(2090)/results.html
are plugin tests.

If you can give me an example test, which you see failing on the bots now with NRWT, or can demonstrate locally as failing with NRWT and passing with ORWT, i'm *very* happy to investigate and fix.

Thank you again for your help and patience!
Comment 23 Simon Fraser (smfr) 2011-10-24 13:40:33 PDT
I looked on the bot, and TestNetscapePlugin.plugin is missing from the build results dir for some reason
Comment 24 Simon Fraser (smfr) 2011-10-24 13:43:30 PDT
ORWT has:

    # FIXME: We build both DumpRenderTree and WebKitTestRunner for
    # WebKitTestRunner runs becuase DumpRenderTree still includes
    # the DumpRenderTreeSupport module and the TestNetscapePlugin.
    # These two projects should be factored out into their own
    # projects.
    buildDumpTool("DumpRenderTree");
    buildDumpTool("WebKitTestRunner") if $useWebKitTestRunner;

Do we need the same in NRWT?
Comment 25 Eric Seidel (no email) 2011-10-24 13:56:22 PDT
I suspect that's exactly the bug, yes.  I'll move that hack into build-webkittestrunner instead of ORWT/NRWT.
Comment 26 Simon Fraser (smfr) 2011-10-24 13:57:48 PDT
Yep, that's it. I filed bug 70759.
Comment 27 Eric Seidel (no email) 2011-10-24 13:58:23 PDT
(In reply to comment #25)
> I suspect that's exactly the bug, yes.  I'll move that hack into build-webkittestrunner instead of ORWT/NRWT.

Filed https://bugs.webkit.org/show_bug.cgi?id=70760.  Will post a patch momentarily.