[Webkit-unassigned] [Bug 162458] EWS should run JavaScriptCore tests

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=162458

--- Comment #35 from Sri Vijayaraghavan <svijayaraghavan at apple.com> ---
(In reply to comment #34)
> 
> Ditto to "getter" comment.
> 
> > Tools/Scripts/webkitpy/common/net/jsctestresults.py:76
> > +        return self.stress_test_failures()
> 
> Ditto to "getter" comment.
> 
> > Tools/Scripts/webkitpy/common/net/jsctestresults.py:79
> > +        return self._test_results
> 
> Ditto to "getter" comment.

failing_test_results(), failing_tests(), and test_results() are all called by other parts of the pre-existing code (for example, the code that posts a comment to Bugzilla containing a list of failing tests, which is _post_reject_message_on_bug()). My objective here was to be able to replace LayoutTestResults with JSCTestResults and have everything else just work.

Getting rid of methods from JSCTestResults will require adding logic elsewhere (eg. the existing code would have to be changed to call test_results() on LayoutTestResults objects, but just look up test_results on JSCTestResults objects). Given that we will have other types of tests in the future I think my method seemed cleaner, but if you're in favor of the other approach please let me know what your reasoning is.

That being said, I'll get rid of the ones that aren't also present in LayoutTestResults.

> > Tools/Scripts/webkitpy/tool/bot/jsctestresultsreader.py:30
> > +class JSCTestResultsReader(object):
> 
> Is this entire class really necessary? All it does it open a file and read
> the contents then pass it through to JSCTestResults. It seems like it'd be
> better to define an @classmethod that took a results_file_path and maybe had
> a name like "results_from_file." But if this is the only entrypoint to
> JSCResults, it might be even better just to merge the functionality of this
> class and the @classmethod from JSCTestResults::results_from_string.

AbstractEarlyWarningSystem requires a reader object which - at the very least - has a .results() method. The situation here is the same - we could either modify the existing code, or have a class that can be easily swapped with an existing class (in this case, LayoutTestResultsReader). Again, I think this is the cleaner tradeoff, but if you prefer the other way please let me know what your reasoning is.

> > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:143
> > +            "Patch was not relevant")
> 
> It doesn't seem necessary for line-length concerns to put each argument on
> its own line. Also, if self._run_command has default arguments then using
> them like: `return self._run_command(args, success_text="Checked relevance
> of patch", failure_text="Patch was not relevant")` would be more clear.
> 
> > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:159
> > +            "Patch does not build")
> 
> Ditto to line-length and default args comment.
> 
> > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:175
> > +            "Unable to build without patch")
> 
> Ditto to line-length and default args comment.
> 
> > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:213
> > +            "Unable to pass tests without patch (tree is red?)")
> 
> Ditto to line-length and default args comment.
I'll make these changes.

> 
> > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:258
> > +        if hasattr(self._delegate, 'group') and self._delegate.group() == "jsc":
> 
> If make_option is used for self._delegate, I don't believe you need the
> hasattr(self._delegate, 'group') statement here.
That is the case if the _delegate is AbstarctEarlyWarningSystem, but there is a whole bunch of other unrelates queues that also use PatchAnalysisTask. I really didn't want to have this, but I couldn't find a way around it that didn't involve adding extra variables to queues that don't need them.

> > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:259
> > +            first_results = self._delegate.test_results()
> 
> I'm unclear on what "first" and "second" are referring to -- can you rename
> the variables to be more accurate?
first_results could be renamed to something like "results_from_first_test_run". I didn't do that because the retry logic for layout tests also uses first_results.

> > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:348
> > +            return True
> 
> This function is very long... is there a way you can nicely put some of the
> logic into more isolated functions?
I'll create separate functions.

> 
> > Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:68
> > +        class TestEWS(AbstractEarlyWarningSystem):
> 
> TestEWS should be defined outside of this function (at the top level of the
> file).
I'll fix this.

> > Tools/Scripts/webkitpy/tool/steps/build.py:71
> > +        if hasattr(self._options, 'group'):
> 
> group should always be defined given you have a default value defined (as
> None).
I''ll fix this.

> > Tools/Scripts/webkitpy/tool/steps/checkpatchrelevance.py:64
> > +        if group == "jsc":
> 
> Given we know we're going to be adding many more projects here, I recommend
> you do something like this:
> 
> # Define group_to_path_mapping at the same scope as jsc_paths
> group_to_paths_mapping = {
>     'jsc': jsc_paths,
> }
> 
> def run(self, state)
>     .....
>     relevant_paths = self.group_to_paths_mapping.get(group, [])
>     relevant = self._does_contain_change_in_paths(change_list,
> relevant_paths)
This is great, I'll add this in!

> > Tools/Scripts/webkitpy/tool/steps/checkpatchrelevance.py:72
> > +        else:
> 
> This else is unnecessary since you return True in the alternative branch.
I''ll fix this.

> > Tools/Scripts/webkitpy/tool/steps/runtests.py:61
> > +        if hasattr(self._options, "group") and self._options.group == "jsc":
> 
> Ditto to hasattr being unnecessary.
I''ll fix this.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170209/69ce60ca/attachment.html>