Bug 195404

Summary: Run unit tests on remote host
Product: WebKit Reporter: Dominik Inführ <dominik.infuehr>
Component: New BugsAssignee: Guillaume Emont <guijemont>
Status: NEW ---    
Severity: Normal CC: annulen, ap, dbates, dewei_zhu, fpizlo, guijemont, jsc32, keith_miller, mark.lam, saam, ticaiolima, xan.lopez, ysuzuki
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Dominik Inführ 2019-03-07 02:00:01 PST
Run unit tests on remote machine
Comment 1 Dominik Inführ 2019-03-07 02:09:06 PST
Created attachment 363860 [details]
Patch
Comment 2 Caio Lima 2019-03-07 05:48:17 PST
Comment on attachment 363860 [details]
Patch

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

LGTM. I think it is a good idea to put on ChangeLog one example of how we can run these tests on command line. It will help a lot for future reference.

> Tools/ChangeLog:9
> +        with cross-compiled builds. Make the unit tests and JS files

I think we can change "Make the unit tests and JS files..." => "To enable remote run, we are making unit tests and JS files...".

> Tools/ChangeLog:14
> +        Avoid using `ldd` for retrieving JSC's dependencies, this does

I would use "..retrieving JSC's dependencies, since this does not work..."

> Tools/ChangeLog:16
> +        dependencies since changes should be quite rare anyway.

I would say: "As an alternative, we hardcode dependencies, since changes there are quite rare."
Comment 3 Dominik Inführ 2019-03-07 07:02:34 PST
Created attachment 363873 [details]
Patch
Comment 4 Alexey Proskuryakov 2019-03-07 15:39:04 PST
This patch implements running JSC tests on a separate device, which we already do for iOS. Is there duplication of functionality here?
Comment 5 Dominik Inführ 2019-03-12 00:02:59 PDT
Yes, run-javascriptcore-tests (or actually run-jsc-stress-tests) runs the JS test files already on the remote machine. However the unit tests (testmasm, testb3, testair, testapi) are always executed locally though. This doesn't work in the case of cross compilation (we build on x64 and then run tests on one of our ARM boards).
Comment 6 Dominik Inführ 2019-03-12 00:05:33 PDT
Created attachment 364361 [details]
Patch
Comment 7 Dominik Inführ 2019-03-18 01:46:27 PDT
Created attachment 365005 [details]
Patch
Comment 8 Dominik Inführ 2019-03-28 06:26:43 PDT
Created attachment 366169 [details]
Patch
Comment 9 Dominik Inführ 2019-03-29 04:48:51 PDT
Updated to also execute testdfg.
Comment 10 Yusuke Suzuki 2019-04-01 11:36:27 PDT
Comment on attachment 366169 [details]
Patch

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

> Tools/Scripts/run-jsc-stress-tests:126
> +$runTestMasm = false
> +$runTestAir = false
> +$runTestB3 = false
> +$runTestDFG = false
> +$runTestAPI = false

Hmm, I don't think this is the correct way. "run-jsc-stress-tests" are scripts for stress tests in JSC, and the stress tests do not include "testmasm" etc. Is it right?
Comment 11 Dominik Inführ 2019-04-02 07:59:26 PDT
I think it makes a lot of sense to implement this in run-jsc-stress-tests since we can reuse all the code that handles remotes, bundles/deploys files and runs binaries on the remote machine.

I agree, run-jsc-stress-tests didn't run testmasm, etc. yet. However one could argue that it already runs more than just stress tests in JSTests/stress. Personally I see run-javascriptcore-tests as a wrapper around run-jsc-stress-tests.
Comment 12 Guillaume Emont 2019-05-30 05:35:13 PDT
@Yusuke: in light of Dominik's last comment, do you still think this should be implemented differently? If yes, do you have a suggestion of what you think the right approach would be?

Also, I think a big argument for doing this in run-jsc-stress-tests is that it's where the whole remote logic is implemented. When parameters to run things remotely are passed to run-javascriptcore-tests, all it does is forward that to run-jsc-stress-tests.
Comment 13 Guillaume Emont 2019-09-26 10:10:24 PDT
Created attachment 379655 [details]
Patch

Updated version of the patch that works with latest upstream and testdfg. Also fixes issues when running unit tests but no js tests on a remote host (run-jsc-stress-tests wouldn't be called in that case).
Comment 14 Guillaume Emont 2019-09-26 10:13:48 PDT
(In reply to Guillaume Emont from comment #13)
> Created attachment 379655 [details]
> Patch
> 
> Updated version of the patch that works with latest upstream and testdfg.
> Also fixes issues when running unit tests but no js tests on a remote host
> (run-jsc-stress-tests wouldn't be called in that case).

...Also, I kept the changes where they are, using run-jsc-stress-tests to run the unit tests. While this is a little unexpected, the alternative seems to be to duplicate the remote testing functionality, by adding a second implementation of it in perl in run-javascriptcore-tests over the existing one in ruby in run-jsc-stress-tests, so I would think that the current solution is preferable.
Comment 15 Caio Lima 2019-09-26 13:09:23 PDT
(In reply to Guillaume Emont from comment #14)
> (In reply to Guillaume Emont from comment #13)
> > Created attachment 379655 [details]
> > Patch
> > 
> > Updated version of the patch that works with latest upstream and testdfg.
> > Also fixes issues when running unit tests but no js tests on a remote host
> > (run-jsc-stress-tests wouldn't be called in that case).
> 
> ...Also, I kept the changes where they are, using run-jsc-stress-tests to
> run the unit tests. While this is a little unexpected, the alternative seems
> to be to duplicate the remote testing functionality, by adding a second
> implementation of it in perl in run-javascriptcore-tests over the existing
> one in ruby in run-jsc-stress-tests, so I would think that the current
> solution is preferable.

How about refactoring the remote execution out of `run-jsc-stress-tests` and share it with a new script to run unit tests? I think this would be a better approach than implementing it into `run-jsc-stress-tests` or duplicating the code into `run-javascriptcore-tests`.
Comment 16 Guillaume Emont 2019-09-27 09:34:25 PDT
(In reply to Caio Lima from comment #15)
> (In reply to Guillaume Emont from comment #14)
> > (In reply to Guillaume Emont from comment #13)
> > > Created attachment 379655 [details]
> > > Patch
> > > 
> > > Updated version of the patch that works with latest upstream and testdfg.
> > > Also fixes issues when running unit tests but no js tests on a remote host
> > > (run-jsc-stress-tests wouldn't be called in that case).
> > 
> > ...Also, I kept the changes where they are, using run-jsc-stress-tests to
> > run the unit tests. While this is a little unexpected, the alternative seems
> > to be to duplicate the remote testing functionality, by adding a second
> > implementation of it in perl in run-javascriptcore-tests over the existing
> > one in ruby in run-jsc-stress-tests, so I would think that the current
> > solution is preferable.
> 
> How about refactoring the remote execution out of `run-jsc-stress-tests` and
> share it with a new script to run unit tests? I think this would be a better
> approach than implementing it into `run-jsc-stress-tests` or duplicating the
> code into `run-javascriptcore-tests`.

I'm giving it a go, though there's a bit of work, since the whole script relies heavily on global variables (and I'm learning ruby as I go).
Comment 17 Guillaume Emont 2019-10-04 05:31:44 PDT
Created attachment 380214 [details]
Patch

I've moved the remote stuff to a new file, but I'm hitting a point where we might need a small discussion: I can either put the code to run remote unit tests in its own ruby script (say, run-jsc-remote-unit-tests), and leave the (perl) code to run unit tests locally as is in run-javascriptcore-tests; or alternatively, I can create a new run-jsc-unit-tests ruby script that would run unit tests both locally and remotely, thus avoiding code duplication. My preference goes towards the second solution that seems cleaner, but I'd like to make sure reviewers agree with me, as this will involve a fair amount of work, as I would port the unit test running from perl (in run-javascriptcore-tests) to ruby (in that new script), and it will be some work to test that things work properly on all platforms after the change.