Bug 190537 - [Tools][webkitpy] fix handling of JSCTESTS_OPTIONS
Summary: [Tools][webkitpy] fix handling of JSCTESTS_OPTIONS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-10-12 14:27 PDT by jsc-armv7 EWS
Modified: 2018-10-12 16:28 PDT (History)
8 users (show)

See Also:


Attachments
Patch (1.60 KB, patch)
2018-10-12 14:31 PDT, Guillaume Emont
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jsc-armv7 EWS 2018-10-12 14:27:39 PDT
In DeprecatedPort.run_javascriptcore_tests_command() JSCTESTS_OPTIONS needs to be split before adding it to the command since it is treated as a list of args.
Comment 1 Guillaume Emont 2018-10-12 14:31:52 PDT
Created attachment 352204 [details]
Patch

The patch.
Comment 2 jsc-armv7 EWS 2018-10-12 14:40:27 PDT
(In reply to Guillaume Emont from comment #1)
> Created attachment 352204 [details]
> Patch
> 
> The patch.

I think (hope) the jsc-armv7 EWS should get green if this works.
Comment 3 jsc-armv7 EWS 2018-10-12 14:59:15 PDT
(In reply to Guillaume Emont (jsc-armv7-ews) from comment #2)
> (In reply to Guillaume Emont from comment #1)
> > Created attachment 352204 [details]
> > Patch
> > 
> > The patch.
> 
> I think (hope) the jsc-armv7 EWS should get green if this works.

...Except webkitpy changes are skipped.
Comment 4 Alexey Proskuryakov 2018-10-12 15:09:20 PDT
Comment on attachment 352204 [details]
Patch

The patch looks good.

I’m not so sure about the idea of passing options via environment. The port object is supposed to implement the knowledge of how to run tools, and passing arguments verbatim violates incapsulation. 

Why was this approach chosen?
Comment 5 jsc-armv7 EWS 2018-10-12 15:26:59 PDT
(In reply to Alexey Proskuryakov from comment #4)
> Comment on attachment 352204 [details]
> Patch
> 
> The patch looks good.
> 
> I’m not so sure about the idea of passing options via environment. The port
> object is supposed to implement the knowledge of how to run tools, and
> passing arguments verbatim violates incapsulation. 
> 
> Why was this approach chosen?

The goal is to be able to pass a --remote-config-file option so that run-javascriptcore-tests runs the test on the correct devices, which are dependent on the machine where it is run and not on the port.
Comment 6 jsc-armv7 EWS 2018-10-12 15:28:34 PDT
(In reply to Guillaume Emont (jsc-armv7-ews) from comment #5)
> (In reply to Alexey Proskuryakov from comment #4)
> > Comment on attachment 352204 [details]
> > Patch
> > 
> > The patch looks good.
> > 
> > I’m not so sure about the idea of passing options via environment. The port
> > object is supposed to implement the knowledge of how to run tools, and
> > passing arguments verbatim violates incapsulation. 
> > 
> > Why was this approach chosen?
> 
> The goal is to be able to pass a --remote-config-file option so that
> run-javascriptcore-tests runs the test on the correct devices, which are
> dependent on the machine where it is run and not on the port.

Also, I'd love if that could be a parameter to webkit-patch <name of ews queue>, but I could not find a simple way to do that, and saw that there was a precedent with MAKEFLAGS.
Comment 7 WebKit Commit Bot 2018-10-12 16:27:55 PDT
Comment on attachment 352204 [details]
Patch

Clearing flags on attachment: 352204

Committed r237089: <https://trac.webkit.org/changeset/237089>
Comment 8 WebKit Commit Bot 2018-10-12 16:27:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2018-10-12 16:28:26 PDT
<rdar://problem/45241227>