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

[bugzilla-daemon at webkit.org]

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

--- Comment #47 from Sri Vijayaraghavan <svijayaraghavan at apple.com> ---
(In reply to comment #42)
> Comment on attachment 301117 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301117&action=review
> 
> > Tools/QueueStatusServer/config/queues.py:2
> > -# Copyright (C) 2014 Apple Inc. All rights reserved.
> > +# Copyright (C) 2014, 2017 Apple Inc. All rights reserved.
> >  # Copyright (C) 2013 Google Inc. All rights reserved.
> 
> This patch is sufficient large. Towards making it straightforward to review
> this code for correctness I would appreciate if you would split this patch
> into three sub-patches: one patch for the changes to QueueStatusServer, one
> patch for non-test webkitpy changes, and one patch for the webkitpy unit
> tests. When it comes time to land the patches we can choose to land each
> sub-patch or combine the sub-patches into one mega patch.
I’ll split up the patch after addressing your suggestions. Should I upload all of them against this bug, or create three bugs?

> [...Define a dedicated status message constant, say skip_status...]
Addressed at the end.

> > Tools/Scripts/webkitpy/common/net/jsctestresults.py:52
> > +            _log.error("Invalid JSON results")
> 
> This patch alternatives between using double-quoted string literals and
> single-quoted string literals. From my understanding we prefer to use
> single-quoted string literals unless the string literal contains a single
> quote (then we use a double-quoted string literal). Regardless, we should
> pick one quoting style and stick with it throughout this patch.
Many existing files use double quotes already. I’ve attempted to keep things consistent within each file, but I’d be happy to use single quotes throughout the patch if you prefer.

> > Tools/Scripts/webkitpy/common/net/jsctestresults.py:77
> > +    def failing_test_results(self):
> > +        return self.results
> > +
> > +    def test_results(self):
> > +        return self.results
> > +
> > +    def failing_tests(self):
> > +        return self.results
> 
> How did you come to the decision to return the same result for all of these
> functions?I know that we designed JSCTestResults to conform to the interface
> of LayoutTestResults but if methods are not applicable for the JSC EWS and
> later changes ensure we do not call these functions then I suggest we omit
> such functions. If we really need these functions then it seems weird that
> they are return the same result.
With JSC, the output from running the tests is just the failures. The layout test output is the archive of all results. This explains why test_results and failing_tests (used in different parts of the rest of the code, such as posting a comment to Bugzilla listing the failures) are different in LayoutTestResults, but the same in JSCTestResults.

It’s a tradeoff between adding “if jsc” conditionals elsewhere in the code (including in methods that this patch doesn’t touch), vs just conforming to the interface here.  I think it’s cleaner to conform to the interface, but if you feel otherwise I’d like to hear your reasoning for it.

What do you mean by “later changes”? If you mean future patches, one of the fixme’s in PatchAnalysisTask is to combine the retry logic JSC and Layout test runs. failing_test_results() isn’t actually required in the current form, but combining the retry logic would probably require it.


(In reply to comment #45)
> Comment on attachment 301437 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301437&action=review
> 
> r-; with the exception of my second remark in comment 42 all other remarks
> in comment 42 were neither addressed in this patch nor replied to with a
> reason for disagreement. Please either address all of my remarks in comment
> 42 in an updated patch or reply to them with your reason(s) for
> disagreement. I have some more questions with this iteration of the patch.
> 
> > Tools/QueueStatusServer/handlers/statusbubble.py:190
> > +        # EWS queues that weren't skipped are also shown when complete.
> 
> I do not feel that this comment adds much value. If you want to keep it then
> at least move it below the if-statement because the code before the
> if-statement and the if-statement itself are for all queues, and non-EWS
> queues, respectively. The code that is applicable for EWS queues is after
> the if-statement (lines 193 thru 195).
I’ll remove the comment.

> > Tools/Scripts/webkitpy/common/config/ports.py:111
> > +        if build_style == "debug":
> > +            command.append("--debug")
> > +        if build_style == "release":
> > +            command.append("--release")
> 
> I know that you are just mimicking the code from
> build_webkit_command/run_webkit_tests_command. We should not duplicate this
> code. We should define a static private helper function, say
> _append_build_style_flag(), that takes a list and a build style and appends
> the appropriate build flag. When we move this code into
> _append_build_style_flag() we should take this opportunity to clean it up
> and write it using an elif to avoid an unnecessary string comparison when
> build_style == 'debug'. Then we can write build_jsc_command,
> build_webkit_command, and run_webkit_tests_command (and
> run_javascriptcore_tests_command?) in terms of _append_build_style_flag().
This is great, I’ll do this.

> > Tools/Scripts/webkitpy/common/config/ports.py:117
> > +        command.append("--json-output=jsc_test_results.json")
> 
> Can we write this as two arguments?
I’m not sure what this would look like in practice - could you give an example of what this might look like?

> > Tools/Scripts/webkitpy/common/config/ports.py:119
> > +        if (build_style == "debug"):
> > +            command.append("--debug")
> 
> How did you come to the decision to only support a debug build style?
The run-javascriptcore-tests script implicitly treats --release as the default option, so passing --release has the same effect as passing no build flag at all. Still, if we’re be using  _append_build_style_flag() in the other methods, I’ll use it here as well.

> > Tools/Scripts/webkitpy/common/config/ports.py:169
> > +    def run_webkit_tests_command(self, build_style=None, test_suite=None):
> > +        if test_suite == "jsc":
> > +            return super(MacPort, self).run_javascriptcore_tests_command(build_style)
> > +
> 
> Where are we invoking run_webkit_tests_command() with a non-None test_suite?
> 
> > Tools/Scripts/webkitpy/tool/bot/jsctestresultsreader.py:38
> > +        except (IOError, KeyError):  # File does not exist or can't be read.
> 
> When is the exception KeyError raised? Unit tests?
Thanks for catching this. 

I added this in when adding unit tests for the behavior when the test results could not be read. In the unit test environment with the mock filesystem, this would throw a KeyError.

However, turns out I later mocked out a little too much, so this line would never actually be hit. I’ll change back the mock methods so it now attempts to read the test results and fails.

> > Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:61
> > +            jsc_results_directory = os.path.dirname(os.path.dirname(os.path.dirname(self._port.results_directory())))
> 
> We should not be burying this directory path here. Similar to how we have a
> Port.result_directory() function, please add a function to the port base
> class and/or derived classes that returns the appropriate path tot he JSC
> results directory.
I’ll work on this.


(In reply to comment #46)
> Comment on attachment 301437 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301437&action=review
> 
> > Tools/QueueStatusServer/model/queuestatus.py:46
> > +        return self.message.startswith(messages.skip_status)
> 
> I just noticed that this disagrees with what I wrote in my second remark in
> comment 42, which implemented this function using a strict string
> comparision. Unless the EWS code does not emit skip_status (what does it
> emit when AbstractPatchQueue.did_fail() is invoked?) when skipping a patch
> by default, I do not feel there is value in having the EWS code (in
> webkitpy) emits a message of the form "Patch not applicable to %s."
I’ll change the message emitted by EWS from “Skip: Patch not applicable to %s” to just “Skip”, which would enable a string equality comparison in QueueStatusServer.

-- 
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/20170215/b44576c8/attachment.html>