Bug 202063 - Tool to mark jsc test skip/enable
Summary: Tool to mark jsc test skip/enable
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zhifei Fang
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-09-20 16:49 PDT by Zhifei Fang
Modified: 2019-10-15 15:45 PDT (History)
7 users (show)

See Also:


Attachments
Patch (11.07 KB, patch)
2019-09-20 16:51 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (11.68 KB, patch)
2019-10-08 13:41 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (11.61 KB, patch)
2019-10-15 11:46 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zhifei Fang 2019-09-20 16:49:59 PDT
A script that can mark jsc test skip under certain conditions.
Comment 1 Zhifei Fang 2019-09-20 16:51:44 PDT
Created attachment 379284 [details]
Patch
Comment 2 Alexey Proskuryakov 2019-09-22 22:49:22 PDT
Comment on attachment 379284 [details]
Patch

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

> Tools/Scripts/run-jsc-stress-tests:187
> +               ['--hardware', GetoptLong::REQUIRED_ARGUMENT],

Is it ok that this is a required argument, but it’s only added conditionally by the caller?

Also, while I don’t have a better suggestion, the argument name feels unusual to me. Is there any precedent for —hardware? Please respond privately if it’s internal.
Comment 3 Alexey Proskuryakov 2019-10-07 13:42:58 PDT
> Is there any precedent for —hardware?

Something with a "model" in the name may be more natural. I guess I don't know what kinds of values are accepted here.
Comment 4 Keith Miller 2019-10-07 14:59:33 PDT
Comment on attachment 379284 [details]
Patch

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

r=me with option name change.

> Tools/Scripts/mark-jsc-stress-test:127
> +    if args["jsc_json_output"]:

Does python convert the - to _? That's bizarre...

>> Tools/Scripts/run-jsc-stress-tests:187
>> +               ['--hardware', GetoptLong::REQUIRED_ARGUMENT],
> 
> Is it ok that this is a required argument, but it’s only added conditionally by the caller?
> 
> Also, while I don’t have a better suggestion, the argument name feels unusual to me. Is there any precedent for —hardware? Please respond privately if it’s internal.

Yeah, hardware is a bit of a weird name in the context of JSC tests since JSC cares about both the platform and the architecture. Can we call this platform instead?
Comment 5 Jonathan Bedard 2019-10-07 15:21:26 PDT
Comment on attachment 379284 [details]
Patch

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

> Tools/Scripts/mark-jsc-stress-test:12
> +formatter = logging.Formatter('%(asctime)s - %(levelname)s - %(message)s')

Does this work in Python 3? Given our current efforts, I'd like new scripts to be compatible.

> Tools/Scripts/mark-jsc-stress-test:17
> +def iter_dir_recusive(path, callback):

Can we not just use os.walk?

> Tools/Scripts/mark-jsc-stress-test:44
> +    def _generate_condition_op(self, value):

I don't think this is a super useful function, it would be more clean to just inline it in _parse_condition

> Tools/Scripts/mark-jsc-stress-test:58
> +                res.append('{} {} "{}"'.format(variable, self._generate_condition_op(word), word))

Don't we need to script the ! from the word if it has one?

> Tools/Scripts/mark-jsc-stress-test:90
> +        # remove the exisiting skip line, so that we can apply the new one

Nit: 'Remove the existing skip line to replace it with a new one

> Tools/Scripts/mark-jsc-stress-test:114
> +    parser_skip.add_argument("--host-os", help="skip if host os matches given value, you can specify complex condition like 'windows or linux' '!windows and !linux'")

We usually describe this as 'platform'

> Tools/Scripts/mark-jsc-stress-test:115
> +    parser_skip.add_argument("--hardware", help="skip if hardware matches given value, you can specify complex condition like 'watch3 or watch4' '!watch3 and !watch4'")

The idiom I've been using for this is 'model'....same idea, but let's keep things standard.

Also, can we be more specific about the types of models specified? I have historically use the simulators as a guide...watches, for example, are canonically 'Apple Watch Series x'

> Tools/Scripts/mark-jsc-stress-test:116
> +    parser_skip.add_argument("--arch", help="skip if architecture matches given value, you can speicify complex condition like 'arm or x86' '!arm and !x86'")

No reason to use an abbreviation here, just say architecture (especially since argparse is smart enough to allow abbreviations)
Comment 6 Zhifei Fang 2019-10-07 15:38:24 PDT
(In reply to Keith Miller from comment #4)
> Comment on attachment 379284 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=379284&action=review
> 
> r=me with option name change.
> 
> > Tools/Scripts/mark-jsc-stress-test:127
> > +    if args["jsc_json_output"]:
> 
> Does python convert the - to _? That's bizarre...
> 
> >> Tools/Scripts/run-jsc-stress-tests:187
> >> +               ['--hardware', GetoptLong::REQUIRED_ARGUMENT],
> > 
> > Is it ok that this is a required argument, but it’s only added conditionally by the caller?
> > 
> > Also, while I don’t have a better suggestion, the argument name feels unusual to me. Is there any precedent for —hardware? Please respond privately if it’s internal.
> 
> Yeah, hardware is a bit of a weird name in the context of JSC tests since
> JSC cares about both the platform and the architecture. Can we call this
> platform instead?

As Jonathan mentioned, usually we call host os to platform.
The hardware here is for a specific hardware that we want to skip tests, for example those devices that haven't support JIT yet, which will run supper slowly for some tests
Comment 7 Zhifei Fang 2019-10-08 13:41:40 PDT
Created attachment 380459 [details]
Patch
Comment 8 Zhifei Fang 2019-10-08 13:46:14 PDT
(In reply to Jonathan Bedard from comment #5)
> Comment on attachment 379284 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=379284&action=review
> 
> > Tools/Scripts/mark-jsc-stress-test:12
> > +formatter = logging.Formatter('%(asctime)s - %(levelname)s - %(message)s')
> 
> Does this work in Python 3? Given our current efforts, I'd like new scripts
> to be compatible.

https://docs.python.org/3/library/logging.html
Yes

> 
> > Tools/Scripts/mark-jsc-stress-test:17
> > +def iter_dir_recusive(path, callback):
> 
> Can we not just use os.walk?
> 
> > Tools/Scripts/mark-jsc-stress-test:44
> > +    def _generate_condition_op(self, value):
> 
> I don't think this is a super useful function, it would be more clean to
> just inline it in _parse_condition
> 
> > Tools/Scripts/mark-jsc-stress-test:58
> > +                res.append('{} {} "{}"'.format(variable, self._generate_condition_op(word), word))
> 
> Don't we need to script the ! from the word if it has one?
> 
> > Tools/Scripts/mark-jsc-stress-test:90
> > +        # remove the exisiting skip line, so that we can apply the new one
> 
> Nit: 'Remove the existing skip line to replace it with a new one
> 
> > Tools/Scripts/mark-jsc-stress-test:114
> > +    parser_skip.add_argument("--host-os", help="skip if host os matches given value, you can specify complex condition like 'windows or linux' '!windows and !linux'")
> 
> We usually describe this as 'platform'
> 
> > Tools/Scripts/mark-jsc-stress-test:115
> > +    parser_skip.add_argument("--hardware", help="skip if hardware matches given value, you can specify complex condition like 'watch3 or watch4' '!watch3 and !watch4'")
> 
> The idiom I've been using for this is 'model'....same idea, but let's keep
> things standard.
> 
> Also, can we be more specific about the types of models specified? I have
> historically use the simulators as a guide...watches, for example, are
> canonically 'Apple Watch Series x'
> 
> > Tools/Scripts/mark-jsc-stress-test:116
> > +    parser_skip.add_argument("--arch", help="skip if architecture matches given value, you can speicify complex condition like 'arm or x86' '!arm and !x86'")
> 
> No reason to use an abbreviation here, just say architecture (especially
> since argparse is smart enough to allow abbreviations)
Comment 9 Jonathan Bedard 2019-10-14 17:23:11 PDT
Comment on attachment 380459 [details]
Patch

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

> Tools/Scripts/mark-jsc-stress-test:18
> +    if os.path.isfile(path) and os.path.splitext(path)[1] == '.js':

Bit confused why we need to recursion here? can't we just put this 'if' statement inside the loop?

Probably should just name this 'iter_directory'

> Tools/Scripts/mark-jsc-stress-test:56
> +    # Translate it to ruby: $hostOs != A or $hostOs == B or $hostOs == C and $hostOs == D 

# Translated to ruby: ...

> Tools/Scripts/mark-jsc-stress-test:114
> +]

Feels like there should be a newline after this.

> Tools/Scripts/mark-jsc-stress-test:118
> +    file_list_help = "files/directories list: a.js, b.js, c.js, use - if you are using --jsc-json-output"

What do you mean by 'use -' if you are using --jsc-json-output?

> Tools/Scripts/mark-jsc-stress-test:119
> +    parser_enable = subparsers.add_parser("enable", help="enable the tests which are marked as skipped by this script")

Capitalize help message

> Tools/Scripts/mark-jsc-stress-test:121
> +    parser_enable.add_argument("--jsc-json-output", help="run-javascriptcore-tests json output, you can use it to enable all the failed tests if they have been set to skip by this script")

Should be something like: "Pass the json output of run-javascriptcore-tests to unskip all failed tests"

Side note: Is that really what you mean? Seems like it should be unskipping all passed tests

> Tools/Scripts/mark-jsc-stress-test:123
> +    parser_skip = subparsers.add_parser("skip", help="insert skip condition to given files/directories")

Capitalize help message

> Tools/Scripts/mark-jsc-stress-test:125
> +    parser_skip.add_argument("--jsc-json-output", help="run-javascriptcore-tests json output, you can use it to skip all the failed tests")

Duplicate?

> Tools/Scripts/mark-jsc-stress-test:126
> +    parser_skip.add_argument("--platform", "--host-os", help="skip if host os matches given value, you can specify complex condition like 'windows or linux' '!windows and !linux'")

Skip if host matches a given condition. Examples: 'windows or linux', '!mac', '!windows and !linux'

> Tools/Scripts/mark-jsc-stress-test:127
> +    parser_skip.add_argument("--model", help="skip if hardware model matches given value, you can specify complex condition like 'watch3 or watch4' '!Apple Watch Series 3 and !Apple Watch Series 4'")

Skip if hardware matches a given condition. Examples: ...

> Tools/Scripts/mark-jsc-stress-test:128
> +    parser_skip.add_argument("--architecture", help="skip if architecture matches given value, you can speicify complex condition like 'arm or x86' '!arm and !x86'")

Skip if architecture matches a given condition. Examples: ...
Comment 10 Zhifei Fang 2019-10-14 18:48:50 PDT
Comment on attachment 380459 [details]
Patch

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

>> Tools/Scripts/mark-jsc-stress-test:18
>> +    if os.path.isfile(path) and os.path.splitext(path)[1] == '.js':
> 
> Bit confused why we need to recursion here? can't we just put this 'if' statement inside the loop?
> 
> Probably should just name this 'iter_directory'

The path arg can be directly a file instead of a directory. Just don't want to repeat my file handling logic, so I call itself.

>> Tools/Scripts/mark-jsc-stress-test:118
>> +    file_list_help = "files/directories list: a.js, b.js, c.js, use - if you are using --jsc-json-output"
> 
> What do you mean by 'use -' if you are using --jsc-json-output?

in this case, you don't need a file name. but still need to give a "-" for file argument.
Comment 11 Zhifei Fang 2019-10-15 11:46:34 PDT
Created attachment 381007 [details]
Patch
Comment 12 WebKit Commit Bot 2019-10-15 15:24:10 PDT
Comment on attachment 381007 [details]
Patch

Clearing flags on attachment: 381007

Committed r251161: <https://trac.webkit.org/changeset/251161>
Comment 13 WebKit Commit Bot 2019-10-15 15:24:12 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2019-10-15 15:45:36 PDT
<rdar://problem/56311570>