Bug 202063 – Tool to mark jsc test skip/enable

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Zhifei Fang]

A script that can mark jsc test skip under certain conditions.

[Zhifei Fang]

Created attachment 379284 [details]
Patch

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

> 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.

[Keith Miller]

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?

[Jonathan Bedard]

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)

[Zhifei Fang]

(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

[Zhifei Fang]

Created attachment 380459 [details]
Patch

[Zhifei Fang]

(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)

[Jonathan Bedard]

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: ...

[Zhifei Fang]

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.

[Zhifei Fang]

Created attachment 381007 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 381007 [details]
Patch

Clearing flags on attachment: 381007

Committed r251161: <https://trac.webkit.org/changeset/251161>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56311570>