Bug 187893 – Add back --wtf-only to run-api-tests

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

[Daniel Bates]

Following the rewrite of run-api-tests in bug #181043 we lost the ability to build and run just the WTF API tests by passing --wtf-only. We should add back the --wtf-only command line option so as to allow developers that iterate quickly on WTF functionality.

[Radar WebKit Bug Importer]

<rdar://problem/42483983>

[Jonathan Bedard]

'run-api-tests --no-build TestWTF' might be a sufficient workaround in the interim.

I would actually prefer to do this implicitly, meaning that 'run-api-tests TestWTF' would figure out that it didn't need to check for or build TestWebKitAPI.

[Daniel Bates]

(In reply to Jonathan Bedard from comment #2)
> 'run-api-tests --no-build TestWTF' might be a sufficient workaround in the
> interim.
> 
> I would actually prefer to do this implicitly, meaning that 'run-api-tests
> TestWTF' would figure out that it didn't need to check for or build
> TestWebKitAPI.

I prefer a flag and not have to remember the name of the underlying executable.

[Jonathan Bedard]

Created attachment 346287 [details]
Patch

[Jonathan Bedard]

On Dan's request, this patch has the flag in it.

This patch still supports the implicit way too. I like the implicit way because it covers API binaries other than TestWTF (Windows, for example, has TestWebCore, TestWTF and TestWebKitLegacy).

[Jonathan Bedard]

Created attachment 346288 [details]
Patch

[Daniel Bates]

Comment on attachment 346288 [details]
Patch

If we are going to go with a more generic approach then I prefer that we also add flags for running TestWebCore and TestWebKitLegacy as well. We should also find some way to convey in the usage text that these options are only applicable to Windows. One way to do this is to take a similar approach as seen in "build-webkit --help"  and "run-webkit-tests --help" and append "(Windows only)" at the end of the help text for each option. Another way to do this is to take an approach similar to "run-webkit-app --help" and group the Windows-specific options under a "Options specific to Windows" group. 

On another note, would passing the name of the binary prohibit us from using the name of the binary as the test case name (e.g. TEST(TestWTF, myTest))?

Additional remarks:
It should not be required for a person to know the names of the test binaries (e.g. TestWebCore). Although it's unlikely that the names of these binaries will change, they can change and it's ludicrous to expect people to have to remember what the names are. We want to make our tools straightforward to work with and let developers get things done. Exposing convenience flags to do this is the most straightforward and unsurprising way to achieve this result.

[Jonathan Bedard]

(In reply to Daniel Bates from comment #7)
> Comment on attachment 346288 [details]
> Patch
> 
> ...
>

I don't think the additional flags are actually useful because we can't skip building those binaries (build-api-tests only supports --wtf-only). Adding these options would also duplicate the list of API test binaries.

I think that you're also losing sight of the existing functionality of run-api-tests that is being enhanced here. It is already the case (without this patch) that a user can pick out a specific test (or group of tests) to be run. So, something like this:

    run-api-tests TestWTF.WTF_WeakPtr.Basic

will only run the WTF_WeakPtr.Basic test inside the TestWTF binary.  Likewise, this:

    run-api-tests TestWTF.WTF_Vector

would only run the WTF_Vector suite inside the TestWTF binary. Your critique that remembering these binaries is not something that an individual can realistically be expected to do has some merit, which is why the current script will attempt to determine which binary a test belongs to if the user does something like this:

    run-api-tests WTF_Vector

But this is also where things start to get a bit ambiguous, because it's unclear if WTF_Vector refers to a binary or a test suite inside a binary. In order to disambiguate this, we first check if its a binary and then check if it's a test suite inside a binary. As is often the case with programs, if the user would like something specific, the user should BE specific. And that's where I have to disagree that expecting a user to know the disambiguated test name that they would like to test is 'ludicrous.'

The more general approach makes sense because if the user was specific (which we already support), we have enough information to skip certain checks. Put another way, if the user does this:

    run-api-tests TestWTF.WTF_Vector

we shouldn't build TestWebKitAPI.

[Jonathan Bedard]

(In reply to Daniel Bates from comment #7)
> Comment on attachment 346288 [details]
> Patch
> 
> ...
> 
> On another note, would passing the name of the binary prohibit us from using
> the name of the binary as the test case name (e.g. TEST(TestWTF, myTest))?
> 
> ...
>

Wanted to separate this comment.

It's already a problem as is. It doesn't prohibit this, exactly, but it means that if you had TestWebKitAPI.TestWTF, the ambiguous case (ie, run-api-tests TestWTF) would assume you meant the binary. 'run-api-tests TestWebKitAPI.TestWTF' would refer to the test case. All of the output of run-api-tests appends the binary name to avoid this ambiguity.

[Daniel Bates]

(In reply to Jonathan Bedard from comment #8)
> (In reply to Daniel Bates from comment #7)
> > Comment on attachment 346288 [details]
> > Patch
> > 
> > ...
> >
> 
> I don't think the additional flags are actually useful because we can't skip
> building those binaries (build-api-tests only supports --wtf-only). 

The flags are useful because they let a person run a subset of tests.

> Adding
> these options would also duplicate the list of API test binaries.

Nobody should be expected to know the names of the binaries in order to run a subset of tests. Exposing flags allows for such blissful ignorance.

> 
> I think that you're also losing sight of the existing functionality of
> run-api-tests that is being enhanced here. It is already the case (without
> this patch) that a user can pick out a specific test (or group of tests) to
> be run. So, something like this:
> 
>     run-api-tests TestWTF.WTF_WeakPtr.Basic

Where is it documented that you can prepend the binary name? I don't see it in --help:

[[
$ Tools/Scripts/run-api-tests --help
Usage: run_api_tests.py [options] [<path>...]

Options:
  -h, --help            show this help message and exit

  Platform options:
    --platform=PLATFORM
                        Platform to use (e.g., "mac-lion")
    --ios-simulator     Alias for --platform=ios-simulator
    --simulator         DEPRECATED alias for --platform=ios-simulator
    --gtk               Alias for --platform=gtk
    --wpe               Alias for --platform=wpe
    --wincairo          Alias for --platform=wincairo

  Configuration options:
    -t CONFIGURATION, --target=CONFIGURATION
                        (DEPRECATED) (default: Debug)
    --debug             Set the configuration to Debug
    --release           Set the configuration to Release
    --64-bit            use 64-bit binaries by default (x86_64 instead of x86)
    --32-bit            use 32-bit binaries by default (x86 instead of x86_64)

  Printing Options:
    -q, --quiet         Run quietly (errors, warnings, and progress only)
    -v, --verbose       Enable verbose printing
    --timestamps        Print timestamps for each logged line

  WebKit Options:
    -g, --guard-malloc  Enable Guard Malloc (OS X only)
    --root=ROOT         Path to a directory containing the executables needed
                        to run tests.

  Testing Options:
    -d, --dump          Dump all test names without running them
    --build             Check to ensure the build is up-to-date (default).
    --no-build          Don't check to see if the build is up-to-date.
    --timeout=TIMEOUT   Number of seconds to wait before a test times out
    --no-timeout        Disable timeouts for all tests
    --child-processes=CHILD_PROCESSES
                        Number of processes to run in parallel.
    --run-singly        Run a separate process for each test
]]

Regardless, as aformentioned above "nobody should be expected to know the names of the binaries in order to run a subset of tests."

> 
> will only run the WTF_WeakPtr.Basic test inside the TestWTF binary. 
> Likewise, this:
> 
>     run-api-tests TestWTF.WTF_Vector
> 
> would only run the WTF_Vector suite inside the TestWTF binary. Your critique
> that remembering these binaries is not something that an individual can
> realistically be expected to do has some merit, which is why the current
> script will attempt to determine which binary a test belongs to if the user
> does something like this:
> 
>     run-api-tests WTF_Vector
> 
> But this is also where things start to get a bit ambiguous, because it's
> unclear if WTF_Vector refers to a binary or a test suite inside a binary. In
> order to disambiguate this, we first check if its a binary and then check if
> it's a test suite inside a binary. As is often the case with programs, if
> the user would like something specific, the user should BE specific. And
> that's where I have to disagree that expecting a user to know the
> disambiguated test name that they would like to test is 'ludicrous.'
> 

Adding the flags solves this problem by moving the responsibility to the human being to tell the program what it should do. The program can then be dumb and just do what its human overlord told it :)

[Daniel Bates]

I just want to point out that this bug is only about adding back --wtf-only. So, if we can just do the minimal amount of work necessary to support this then I would be happy. We can defer adding additional flags to another bug.

[EWS Watchlist]

Comment on attachment 346288 [details]
Patch

Attachment 346288 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8728764

New failing tests:
http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html

[EWS Watchlist]

Created attachment 346351 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Jonathan Bedard]

(In reply to Daniel Bates from comment #10)
> ...
> 
> Where is it documented that you can prepend the binary name? I don't see it
> in --help:
>
> ...
>

The test names with the prepended binary names are the default behavior. When tests are run (or -d is used), that's what gets printed. It's actually the reverse, the test name without the prepended binary name is the undocumented supported option.

>
> ...
> 
> Regardless, as aformentioned above "nobody should be expected to know the
> names of the binaries in order to run a subset of tests."
> ...
>

I don't agree with this statement. It makes dealing with API tests much harder and would require us to enforce that a specific test name be present in only one of the binaries. We can dodge this requirement by having the test name include the name of the binary (so it's guaranteed to be unique!).

>
> ...
> Adding the flags solves this problem by moving the responsibility to the
> human being to tell the program what it should do. The program can then be
> dumb and just do what its human overlord told it :)

The flags just make picking which binary to run easier. They can't help with running a subset of tests within a binary.

My comments here have been an attempt to explain the implementation choices I've made in the uploaded patch. That patch contains the flag for wtf, but not the other binaries

[Daniel Bates]

(In reply to Jonathan Bedard from comment #14)
> (In reply to Daniel Bates from comment #10)
> > ...
> > 
> > Where is it documented that you can prepend the binary name? I don't see it
> > in --help:
> >
> > ...
> >
> 
> The test names with the prepended binary names are the default behavior.
> When tests are run (or -d is used), that's what gets printed. It's actually
> the reverse, the test name without the prepended binary name is the
> undocumented supported option.
> 

I do not like this new behavior and this behavior does not match the behavior of the previous run-api-tests (before <https://trac.webkit.org/changeset/230998>). I downloaded the latest version of run-api-tests before r230998, <https://trac.webkit.org/changeset/226395>) into Tools/Scripts/old-run-api-tests and compared the output to the current version of run-api-tests. Here are my results:

== Passing test ==

Before r230998:

[[
$ Tools/Scripts/old-run-api-tests --debug --no-build --verbose WTF.StringViewStripLeadingAndTrailingMatchedCharacters
PASS WTF.StringViewStripLeadingAndTrailingMatchedCharacters
]]

After r230998:

[[
$ Tools/Scripts/run-api-tests --debug --no-build --verbose WTF.StringViewStripLeadingAndTrailingMatchedCharacters
Checking build ...
"perl Tools/Scripts/webkit-build-directory --configuration --debug --mac" took 0.41s
Collecting tests ...
"/Volumes/Data/WebKitDev/OpenSource/WebKitBuild/Debug/TestWTF --gtest_list_tests" took 0.16s
"/Volumes/Data/WebKitDev/OpenSource/WebKitBuild/Debug/TestWebKitAPI --gtest_list_tests" took 0.23s
Found 1 tests
Running tests
Sharding tests ...
worker/0 starting
TestWTF.WTF.StringViewStripLeadingAndTrailingMatchedCharacters Passed
worker/0 exiting
Ran 1 tests of 1 with 1 successful
------------------------------
All tests successfully passed!
Testing completed, Exit status: 0
]]

== Failing test ==

Before r230998:

[[
$ Tools/Scripts/old-run-api-tests --debug --no-build --verbose WTF.StringViewStripLeadingAndTrailingMatchedCharacters
FAIL WTF.StringViewStripLeadingAndTrailingMatchedCharacters

/Volumes/Data/WebKitDev/OpenSource/Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:958
Value of: stringViewFromLiteral("AAABBAAA").stripLeadingAndTrailingMatchedCharacters(isA) == stringViewFromLiteral("BBB")
  Actual: false
Expected: true


Tests that failed:
  WTF.StringViewStripLeadingAndTrailingMatchedCharacters
]]

After r230998:

[[
$ Tools/Scripts/run-api-tests --debug --no-build --verbose WTF.StringViewStripLeadingAndTrailingMatchedCharacters
Checking build ...
"perl Tools/Scripts/webkit-build-directory --configuration --debug --mac" took 0.39s
Collecting tests ...
"/Volumes/Data/WebKitDev/OpenSource/WebKitBuild/Debug/TestWTF --gtest_list_tests" took 0.17s
"/Volumes/Data/WebKitDev/OpenSource/WebKitBuild/Debug/TestWebKitAPI --gtest_list_tests" took 0.20s
Found 1 tests
Running tests
Sharding tests ...
worker/0 starting
    
    /Volumes/Data/WebKitDev/OpenSource/Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:958
    Value of: stringViewFromLiteral("AAABBAAA").stripLeadingAndTrailingMatchedCharacters(isA) == stringViewFromLiteral("BBB")
      Actual: false
    Expected: true
    
TestWTF.WTF.StringViewStripLeadingAndTrailingMatchedCharacters Failed
worker/0 exiting
Ran 1 tests of 1 with 0 successful
------------------------------
Test suite failed

Failed

    TestWTF.WTF.StringViewStripLeadingAndTrailingMatchedCharacters
        
        /Volumes/Data/WebKitDev/OpenSource/Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:958
        Value of: stringViewFromLiteral("AAABBAAA").stripLeadingAndTrailingMatchedCharacters(isA) == stringViewFromLiteral("BBB")
          Actual: false
        Expected: true
        

Testing completed, Exit status: 3
]]

As you can see from the above output we did not prepend the binary name (TestWTF) to the test before r230998.

> >
> > ...
> > 
> > Regardless, as aformentioned above "nobody should be expected to know the
> > names of the binaries in order to run a subset of tests."
> > ...
> >
> 
> I don't agree with this statement. It makes dealing with API tests much
> harder 

Yes, it makes it harder for me to deal with API tests.

> and would require us to enforce that a specific test name be present
> in only one of the binaries. We can dodge this requirement by having the
> test name include the name of the binary (so it's guaranteed to be unique!).
> 

I am unclear how this a problem and didn't have this requirement in the old run-api-tests (before r230998). Why do we have this requirement now?

> >
> > ...
> > Adding the flags solves this problem by moving the responsibility to the
> > human being to tell the program what it should do. The program can then be
> > dumb and just do what its human overlord told it :)
> 
> The flags just make picking which binary to run easier. They can't help with
> running a subset of tests within a binary.
> 

The output of TestWTF and TestWebKitAPI disagrees. Here is a truncated portion of the output I get when I run TestWTF:

[[
$ Tools/Scripts/run-webkit-app --debug WebKitBuild/Debug/TestWTF
Starting TestWTF with DYLD_FRAMEWORK_PATH set to point to built WebKit in /Volumes/.../OpenSource/WebKitBuild/Debug.
**PASS** WTF.AtomicStringCreationFromLiteral
**PASS** WTF.AtomicStringCreationFromLiteralUniqueness
**PASS** WTF.AtomicStringExistingHash
**PASS** WTF.AtomicStringNumberDouble
**PASS** WTF.Checked_int8_t
**PASS** WTF.Checked_int16_t
**PASS** WTF.Checked_int32_t
**PASS** WTF.Checked_uint32_t
...
]]

It runs only the WTF tests.

> My comments here have been an attempt to explain the implementation choices
> I've made in the uploaded patch. That patch contains the flag for wtf, but
> not the other binaries

[Daniel Bates]

(In reply to Daniel Bates from comment #15)
> (In reply to Jonathan Bedard from comment #14)
> > (In reply to Daniel Bates from comment #10)
> > > ...
> > > 
> > > Where is it documented that you can prepend the binary name? I don't see it
> > > in --help:
> > >
> > > ...
> > >
> > 
> > The test names with the prepended binary names are the default behavior.
> > When tests are run (or -d is used), that's what gets printed. It's actually
> > the reverse, the test name without the prepended binary name is the
> > undocumented supported option.
> > 
> 
> I do not like this new behavior and this behavior does not match the
> behavior of the previous run-api-tests (before
> <https://trac.webkit.org/changeset/230998>).


Filed bug #188261 for this issue.

> > [...] 
> > My comments here have been an attempt to explain the implementation choices
> > I've made in the uploaded patch. That patch contains the flag for wtf,

> > but not the other binaries

Filed bug #188262 to add convenience flags to run only the WebCore and WebKit Legacy tests on Windows.

[Daniel Bates]

(In reply to Daniel Bates from comment #15)
> [...]
> > >
> > > ...
> > > Adding the flags solves this problem by moving the responsibility to the
> > > human being to tell the program what it should do. The program can then be
> > > dumb and just do what its human overlord told it :)
> > 
> > The flags just make picking which binary to run easier. They can't help with
> > running a subset of tests within a binary.
> > 
> 
> The output of TestWTF and TestWebKitAPI disagrees. Here is a truncated
> portion of the output I get when I run TestWTF:
> 
> [[
> $ Tools/Scripts/run-webkit-app --debug WebKitBuild/Debug/TestWTF
> Starting TestWTF with DYLD_FRAMEWORK_PATH set to point to built WebKit in
> /Volumes/.../OpenSource/WebKitBuild/Debug.
> **PASS** WTF.AtomicStringCreationFromLiteral
> **PASS** WTF.AtomicStringCreationFromLiteralUniqueness
> **PASS** WTF.AtomicStringExistingHash
> **PASS** WTF.AtomicStringNumberDouble
> **PASS** WTF.Checked_int8_t
> **PASS** WTF.Checked_int16_t
> **PASS** WTF.Checked_int32_t
> **PASS** WTF.Checked_uint32_t
> ...
> ]]
> 
> It runs only the WTF tests.
> 

I misread your response. You were claiming we couldn't run a subset of tests in a binary. This is not true. We can do this and here is an example of running just the WTF AtomicString prefixed tests:

[[
$ Tools/Scripts/run-webkit-app --debug WebKitBuild/Debug/TestWTF --gtest_filter=WTF.AtomicString*
Starting TestWTF with DYLD_FRAMEWORK_PATH set to point to built WebKit in /Volumes/Data/WebKitDev/OpenSource/WebKitBuild/Debug.
**PASS** WTF.AtomicStringCreationFromLiteral
**PASS** WTF.AtomicStringCreationFromLiteralUniqueness
**PASS** WTF.AtomicStringExistingHash
**PASS** WTF.AtomicStringNumberDouble
]]

[Jonathan Bedard]

(In reply to Daniel Bates from comment #17)
> (In reply to Daniel Bates from comment #15)
> > [...]
> > > >
> > > > ...
> > > > Adding the flags solves this problem by moving the responsibility to the
> > > > human being to tell the program what it should do. The program can then be
> > > > dumb and just do what its human overlord told it :)
> > > 
> > > The flags just make picking which binary to run easier. They can't help with
> > > running a subset of tests within a binary.
> > > 
> > 
> > The output of TestWTF and TestWebKitAPI disagrees. Here is a truncated
> > portion of the output I get when I run TestWTF:
> > 
> > [[
> > $ Tools/Scripts/run-webkit-app --debug WebKitBuild/Debug/TestWTF
> > Starting TestWTF with DYLD_FRAMEWORK_PATH set to point to built WebKit in
> > /Volumes/.../OpenSource/WebKitBuild/Debug.
> > **PASS** WTF.AtomicStringCreationFromLiteral
> > **PASS** WTF.AtomicStringCreationFromLiteralUniqueness
> > **PASS** WTF.AtomicStringExistingHash
> > **PASS** WTF.AtomicStringNumberDouble
> > **PASS** WTF.Checked_int8_t
> > **PASS** WTF.Checked_int16_t
> > **PASS** WTF.Checked_int32_t
> > **PASS** WTF.Checked_uint32_t
> > ...
> > ]]
> > 
> > It runs only the WTF tests.
> > 
> 
> I misread your response. You were claiming we couldn't run a subset of tests
> in a binary. This is not true. We can do this and here is an example of
> running just the WTF AtomicString prefixed tests:
> 
> [[
> $ Tools/Scripts/run-webkit-app --debug WebKitBuild/Debug/TestWTF
> --gtest_filter=WTF.AtomicString*
> Starting TestWTF with DYLD_FRAMEWORK_PATH set to point to built WebKit in
> /Volumes/Data/WebKitDev/OpenSource/WebKitBuild/Debug.
> **PASS** WTF.AtomicStringCreationFromLiteral
> **PASS** WTF.AtomicStringCreationFromLiteralUniqueness
> **PASS** WTF.AtomicStringExistingHash
> **PASS** WTF.AtomicStringNumberDouble
> ]]

You're responses aren't making much sense. I was talking about the flags on run-api-tests, not the flags on TestWTF.

My claim is that using flags on run-api-tests (such as --only-wtf) doesn't help us with running a subset of tests within a binary, which is why the specific flags aren't really useful in resolving the binary/suite name ambiguity problem.

We support running specific suites and even specific tests through the positional arguments of run-api-tests, that's the whole functionality I'm piggy-backing off of.

[Jonathan Bedard]

(In reply to Daniel Bates from comment #15)
> (In reply to Jonathan Bedard from comment #14)
> > (In reply to Daniel Bates from comment #10)
> > > ...
> > > 
> > > Where is it documented that you can prepend the binary name? I don't see it
> > > in --help:
> > >
> > > ...
> > >
> > 
> > The test names with the prepended binary names are the default behavior.
> > When tests are run (or -d is used), that's what gets printed. It's actually
> > the reverse, the test name without the prepended binary name is the
> > undocumented supported option.
> > 
> 
> I do not like this new behavior and this behavior does not match the
> behavior of the previous run-api-tests (before
> <https://trac.webkit.org/changeset/230998>). I downloaded the latest version
> of run-api-tests before r230998, <https://trac.webkit.org/changeset/226395>)
> into Tools/Scripts/old-run-api-tests and compared the output to the current
> version of run-api-tests. Here are my results:
> ...
>

This behavior is different. But, the previous behavior was ambiguous. So yes, the behavior was changed to remove the ambiguity.

> 
> > >
> > > ...
> > > 
> > > Regardless, as aformentioned above "nobody should be expected to know the
> > > names of the binaries in order to run a subset of tests."
> > > ...
> > >
> > 
> > I don't agree with this statement. It makes dealing with API tests much
> > harder 
> 
> Yes, it makes it harder for me to deal with API tests.
> 

Does the addition of the --wtf-only flag not resolve your concern?

[Jonathan Bedard]

Created attachment 346423 [details]
Patch

[Jonathan Bedard]

I had a discussion in person with Dan Bates today (8/2).

My understanding of the conclusion we reached was to retain the perpending of the binary names as an option, but to expose flags so that engineers don't need to remember the names of those binaries (covered in https://bugs.webkit.org/show_bug.cgi?id=188262).

Additionally, I'm going to improve the help message of run-api-tests since it isn't clear on how test matching with the positional arguments works (covered in https://bugs.webkit.org/show_bug.cgi?id=188280)

[Daniel Bates]

Comment on attachment 346423 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346423&action=review

> Tools/Scripts/webkitpy/port/win.py:65
> +    API_TEST_BINARIES = ['TestWTF', 'TestWebCore', 'TestWebKitLegacy']

These are not the name of the binaries. The binaries end with .exe on Windows. I just realized we are not prepending binary names. We are prepending a canonicalized binary name. This “prepending” feature just sounds more and more like an arbitrary design decision that provides questionable value and unnecessarily complicates the code. I suggest that we remove it. If we cannot then it would simplify the implementation if we could just use the bi

[Daniel Bates]

(In reply to Daniel Bates from comment #22)
> Comment on attachment 346423 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=346423&action=review
> 
> > Tools/Scripts/webkitpy/port/win.py:65
> > +    API_TEST_BINARIES = ['TestWTF', 'TestWebCore', 'TestWebKitLegacy']
> 
> These are not the name of the binaries. The binaries end with .exe on
> Windows. I just realized we are not prepending binary names. We are
> prepending a canonicalized binary name. This “prepending” feature just
> sounds more and more like an arbitrary design decision that provides
> questionable value and unnecessarily complicates the code. I suggest that we
> remove it. If we cannot then it would simplify the implementation if we
> could just use the bi

if we could just use the actual binary name (e.g. TestWebCore.exe). If this is not possible, then the next opportunity for simplification would be to have an array of actual binary names and a mapping function/an array/dictionary for the canonicalized binary names. Each of these simplifications make the code more readable and less fragile.

[Daniel Bates]

Anyone else have an opinion on whether there is value supporting;

./run-WebKit-api TestWebCore

And emitting the test name as something of the form:

TestWebCore.TestSuite.TestNams

?

Supporting this behavior complicates the feature request in this bug to add back --wtf-only because we need to associate this flag with the binary TestWTF (TestWTF.exe on Windows) in order to both pass the right flag to build-api-test to build TestWTF as well as have the Python code check that TestWTF (or whatever canonicalized binary name you passed on the command line) exists. That is, we can’t fix this bug by just adding three if statements: one to pass the right flag to build-api-tests (only if —wtf-only was specified to run-api-tests), one to ensure TestWTF exists, and one to use the TestWTF binary when running tests.

[Jonathan Bedard]

Comment on attachment 346423 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346423&action=review

>>> Tools/Scripts/webkitpy/port/win.py:65
>>> +    API_TEST_BINARIES = ['TestWTF', 'TestWebCore', 'TestWebKitLegacy']
>> 
>> These are not the name of the binaries. The binaries end with .exe on Windows. I just realized we are not prepending binary names. We are prepending a canonicalized binary name. This “prepending” feature just sounds more and more like an arbitrary design decision that provides questionable value and unnecessarily complicates the code. I suggest that we remove it. If we cannot then it would simplify the implementation if we could just use the bi
> 
> if we could just use the actual binary name (e.g. TestWebCore.exe). If this is not possible, then the next opportunity for simplification would be to have an array of actual binary names and a mapping function/an array/dictionary for the canonicalized binary names. Each of these simplifications make the code more readable and less fragile.

It's not an 'arbitrary' design decision, it was a design decision made because regardless of whether we expose the reality of the binaries to the user, the internals of the script need to keep track of which test is associated to which binary. It's much easier if this internal state uses the canonicalized binary names since this means that Windows, Mac and iOS can all share exactly the same code up until they need to actually run the binary.

This patch actually does the inverse of what you're suggesting: def path_to_api_test_binaries() returns a mapping of canonicalized binary names to full binary paths (note that just the binary names is not helpful anywhere, you either need the full path or the canonicalized name). Perhaps we need to eliminate the API_TEST_BINARIES array and just have path_to_api_test_binaries.

[Jonathan Bedard]

Created attachment 346668 [details]
Patch

[Aakash Jain]

I'm not an expert in api tests. It seems that the latest patch addresses most of the concerns. e.g.: This patch implements the feedback: "next opportunity for simplification would be to have an array of actual binary names and a mapping function/an array/dictionary for the canonicalized binary names".


> Anyone else have an opinion on whether there is value supporting;
> ./run-WebKit-api TestWebCore
> 
> And emitting the test name as something of the form:
> TestWebCore.TestSuite.TestNams
I think it is still worth supporting this option, for people who actually remember the binary name. Although it's true that we shouldn't assume people to remember binary name, it would be nice to support this for people who do remember the binary names and want to use it. I don't have a very strong opinion on this though.

[Aakash Jain]

r=me, if Daniel Bates doesn't have strong objection on latest patch.

[Daniel Bates]

(In reply to Aakash Jain from comment #28)
> r=me, if Daniel Bates doesn't have strong objection on latest patch.

I don't.

[Jonathan Bedard]

Created attachment 347381 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 347381 [details]
Patch

Clearing flags on attachment: 347381

Committed r234997: <https://trac.webkit.org/changeset/234997>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.