Bug 37631 – new-run-webkit-tests should not assume the test driver is called "DumpRenderTree"

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Dirk Pranke]

In https://bugs.webkit.org/show_bug.cgi?id=37371 , abarth reported (correctly) that "test_shell" means nothing to the webkit.org community, and renamed everything to DumpRenderTree (or dump_render_tree).

This has three problems. The first is that DumpRenderTree equally means nothing to non-webkitters (or, at least not as much). If we had to choose a single name, I could admit that picking DumpRenderTree is better than test_shell just in terms of familiarity in the community (although, like several other changes that have been landing lately, I fear that these are being done at the risk of confusing the people in Chromium that do know this code the best and yet perhaps aren't being kept in the loop - people like tc@, ojan@, victorw@, pamg@, etc., all copied here).

The second is that if you happen to be testing multiple platforms that do use different names, using an inaccurate name is confusing. I was thrown when, the first time after that patch landed (and I didn't realize it) suddenly my chromium regressions were mentioning DumpRenderTree; I was worried that I was using the wrong command line switches. Of course, even Chromium isn't consistent here (test_shell on Win and Linux, TestShell on the Mac).

So, we should make the user-visible name get pulled from the Port.

The third problem -- as the other bug also mentioned -- is that both DumpRenderTree and test_shell aren't great names (which is why I used Driver as the class name, which admittedly also isn't a great name). Given that the script uses a standard master-slave design, perhaps we should call these "slaves", "workers", or "children" instead? I notice that you at least renamed --num-test-shells to --child-processes, so maybe we should use that instead?

[Adam Barth]

1) We can change the UI if you like.

2) We have a team killing test_shell and replacing it with DumpRenderTree for the Chromium port, so this problem will solve itself naturally.

[Dirk Pranke]

(In reply to comment #1)
> 1) We can change the UI if you like.
> 

Yeah, I'm about to submit a patch to do so :)

> 2) We have a team killing test_shell and replacing it with DumpRenderTree for
> the Chromium port, so this problem will solve itself naturally.

Yes, I know. I look forward to it, but would like to avoid confusion (not the least of which is my own) in the meantime.

[Tony Chang]

FWIW, I'm in support of just calling in DumpRenderTree (or DRT).  Chromium devs can learn to adapt.

Maybe we should have a gyp step that copies test_shell to DumpRenderTree :)

[Dirk Pranke]

As long as DumpRenderTree has the ability to generate text-only diffs (which are prefered) and pixel dumps (which are sometimes necessary), I continue to think its name should be changed :)

[Dirk Pranke]

Created attachment 53401 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 53401 [details]
Patch

ok.

[Dirk Pranke]

Comment on attachment 53401 [details]
Patch

Clearing flags on attachment: 53401

Committed r57672: <http://trac.webkit.org/changeset/57672>

[Dirk Pranke]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/57672 might have broken Tiger Intel Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/57672
http://trac.webkit.org/changeset/57673
http://trac.webkit.org/changeset/57674
http://trac.webkit.org/changeset/57675
http://trac.webkit.org/changeset/57676