Bug 204091 – results.webkit.org: Report JSC tests to the results database

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

Add support to run-javascriptcore-tests to report test failures to the results database.

[Jonathan Bedard]

<rdar://problem/49778900>

[Jonathan Bedard]

Created attachment 383328 [details]
Patch

[Jonathan Bedard]

The lack of a standardized output format and the fact that this script is in perl is quite unfortunate. I'm glad that the results database is dynamic enough that a perl script can talk to it, but this script as a whole concerns me....I think we need to migrate it to Python some time in the next year, otherwise we're going to keep piling up technical debt (like this change).

[Alexey Proskuryakov]

Comment on attachment 383328 [details]
Patch

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

A few comments since I opened this anyway, but not attempting a real review.

> Tools/ChangeLog:12
> +        (runTest): Parse binary output to mark individual tests as passing

I feel like this is an improvement that we could use when presenting results on EWS and bot watcher's dashboard. I am not 100% sure what we do for those now, but we definitely didn't split full results per test before.

> Tools/ChangeLog:263
> [phone]  Dean Jackson  <[e-mail]>

Something bad happened to the ChangeLog.

> Tools/Scripts/run-javascriptcore-tests:537
> +# FIXME: report stress test failureshttps://bugs.webkit.org/show_bug.cgi?id=204096

Space.

[Jonathan Bedard]

Created attachment 383364 [details]
Patch

[Aakash Jain]

Comment on attachment 383364 [details]
Patch

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

> Tools/Scripts/run-javascriptcore-tests:433
> +    open(TEST, "-|", "@command 2>&1") or $testResult = 1;

This seems like a significant change in running these tests. How much testing has been done on it?

> Tools/Scripts/run-javascriptcore-tests:440
> +        #     O2: testRotRWithImmShift(uint64-max, uint32-min): OK!

It seems weird to have 5 spaces here (and other comments below). Maybe just have one space and start with 'e.g.:'

> Tools/Scripts/run-javascriptcore-tests:475
> +        } else {

How do we know if we have covered all the situations?

> Tools/Scripts/run-javascriptcore-tests:763
> +sub uploadConfiguration()

This name seems misleading, this function doesn't upload anything. It simply returns configuration. Should be named something like getConfiguration/getConfigurationToUpload/configurationToUpload etc.

> Tools/Scripts/run-javascriptcore-tests:780
> +        die "No watchOS version specified" if !$version;

is die the right thing to do because version wasn't specified, and that too at the end of the script, when all the tests have already run? If we want the version to be mandatory, this check should be at the beginning of the script to cause early-exit.

> Tools/Scripts/run-javascriptcore-tests:818
> +        die "Cannot determine platform\n";

is die the right thing to do in case of issue in determining configuration? should it be a hard failure? can we default to something?

> Tools/Scripts/run-javascriptcore-tests:842
> +    if (not defined $report) {

Would be good to have a log here.

> Tools/Scripts/run-javascriptcore-tests:887
> +        return 1;

will this be the exit code of this script eventually (since this is the last function to be called in this script)? if so exit code of 1 for success looks incorrect.

> Tools/Scripts/run-javascriptcore-tests:890
> +    return 0;

Ditto about exit code.

[Jonathan Bedard]

Comment on attachment 383364 [details]
Patch

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

>> Tools/Scripts/run-javascriptcore-tests:433
>> +    open(TEST, "-|", "@command 2>&1") or $testResult = 1;
> 
> This seems like a significant change in running these tests. How much testing has been done on it?

This bit isn't actually that significant.

We're basically just consuming the stdout of the testing binaries instead of feeding them straight to the stdout of the harness. I ran jsc tests on my machine, but not much more than that.

>> Tools/Scripts/run-javascriptcore-tests:475
>> +        } else {
> 
> How do we know if we have covered all the situations?

We sort of don't.

jsc tests don't really have a standard format....I basically added regexes and logged the bits which weren't matching until I matched everything that I would call an individual test. This is complicated by the fact that jsc tests have bits where a bunch of cases are combined into a single test, I try to treat those as a single test instead of the dozen or so test bits.

>> Tools/Scripts/run-javascriptcore-tests:780
>> +        die "No watchOS version specified" if !$version;
> 
> is die the right thing to do because version wasn't specified, and that too at the end of the script, when all the tests have already run? If we want the version to be mandatory, this check should be at the beginning of the script to cause early-exit.

We don't really have a reasonable back-up, so I think we sort of have to die.

Good point about the beginning of the script, though....I will call this function early on so we trigger any early exits before running tests.

>> Tools/Scripts/run-javascriptcore-tests:887
>> +        return 1;
> 
> will this be the exit code of this script eventually (since this is the last function to be called in this script)? if so exit code of 1 for success looks incorrect.

I guess this would make more sense in exit-code format.

I wasn't going to make it define the exit code, since that approach did not seem popular when applied to layout tests.

[Jonathan Bedard]

Created attachment 383456 [details]
Patch

[Zhifei Fang]

Comment on attachment 383456 [details]
Patch

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

> Tools/Scripts/run-javascriptcore-tests:782
> +    } elsif (isGtk()) {

We need tvOS as well

> Tools/Scripts/run-javascriptcore-tests:850
> +        suite => 'javascriptcore-tests',

Not sure if we should separate test binaries as different suite

[Jonathan Bedard]

(In reply to Zhifei Fang from comment #9)
> Comment on attachment 383456 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383456&action=review
> 
> > Tools/Scripts/run-javascriptcore-tests:782
> > +    } elsif (isGtk()) {
> 
> We need tvOS as well
> 
> > Tools/Scripts/run-javascriptcore-tests:850
> > +        suite => 'javascriptcore-tests',
> 
> Not sure if we should separate test binaries as different suite

I think no. When you look at https://results.webkit.org/suites, our suites tend to be much higher level than test binaries. In fact, API tests use a similar model to the one we're using here. Actually, I have a bug somewhere about supporting partial suite results, which would probably be the right approach here.

[Aakash Jain]

Comment on attachment 383456 [details]
Patch

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

> Tools/Scripts/run-javascriptcore-tests:376
> +configurationForUpload() if (defined($report));

This should be called only if appropriate parameters are passed to upload the data. We probably don't want to exit the script here (e.g.: in a manual run) if we aren't going to upload anything anyways.

> Tools/Scripts/run-javascriptcore-tests:504
>          writeJsonDataIfApplicable();

Do you want to uploadResults here as well, in case of fail-fast? 
It would probably contain result of tests from single binary, but it might be useful to have that data.

> Tools/Scripts/run-javascriptcore-tests:539
> +# FIXME: report stress test failureshttps://bugs.webkit.org/show_bug.cgi?id=204096

Nit: missing space in failureshttp

> Tools/Scripts/run-javascriptcore-tests:814
> +            } elsif ($version->{minor} eq 14) {

Did you mean 13 here?

> Tools/Scripts/run-javascriptcore-tests:815
> +                $versionName = "High Sierra";

I think we should have few more OSes in this list, like Sierra, El Capitan. Also, should there be a default?

> Tools/Scripts/run-javascriptcore-tests:843
> +    if (not defined $report) {

This seems like quite late to find this issue, we should do an early check of all the parameters, before running the tests.

> Tools/Scripts/run-javascriptcore-tests:868
> +    if (defined $buildbotMaster and defined $builderName and defined $buildNumber and defined $buildbotWorker) {

There should be else case as well to log the error. We should validate the arguments in beginning to ensure that any required parameter is not missing. Maybe add a validateUploadParameters/validateParameters method.

> Tools/Scripts/run-javascriptcore-tests:876
> +    if (defined $ENV{'RESULTS_SERVER_API_KEY'}) {

I believe API key is mandatory for uploading the data. We should check it in beginning and fail early with proper error message if this value is not found.  Maybe add a validateUploadParameters/validateParameters method.

> Tools/Scripts/run-javascriptcore-tests:884
> +    print HANDLE "$encodedUpload\n\0";

How does the output look like?

> Tools/Scripts/run-javascriptcore-tests:892
> +    return 1;

Can we have another parameter to this script (like --silent-upload-failure / --ignore-upload-failure) which would prevent upload failure from being a hard failure. i.e.: even if upload fails don't mark the script as failed. It would be useful for EWS since the upload can fail due to various reasons (e.g. network issue), and we don't want to have any impact of that failure on patch being processed in EWS.

[Jonathan Bedard]

Created attachment 383471 [details]
Patch

[Jonathan Bedard]

Comment on attachment 383456 [details]
Patch

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

>> Tools/Scripts/run-javascriptcore-tests:376
>> +configurationForUpload() if (defined($report));
> 
> This should be called only if appropriate parameters are passed to upload the data. We probably don't want to exit the script here (e.g.: in a manual run) if we aren't going to upload anything anyways.

That's exactly what the if (defined($report)) is doing

>>> Tools/Scripts/run-javascriptcore-tests:782
>>> +    } elsif (isGtk()) {
>> 
>> We need tvOS as well
> 
> I think no. When you look at https://results.webkit.org/suites, our suites tend to be much higher level than test binaries. In fact, API tests use a similar model to the one we're using here. Actually, I have a bug somewhere about supporting partial suite results, which would probably be the right approach here.

Great fun. Bugzilla put the comment in the wrong place. That comment was supposed to be about suites, I will be adding tvOS here.

>> Tools/Scripts/run-javascriptcore-tests:815
>> +                $versionName = "High Sierra";
> 
> I think we should have few more OSes in this list, like Sierra, El Capitan. Also, should there be a default?

I can add more, but why? We only run -2 in automation, we shouldn't be support Sierra and El Capitan anywhere.

I don't think we should have a default, it's OK to report without a version name, so being correct is more important than defining something.

>> Tools/Scripts/run-javascriptcore-tests:843
>> +    if (not defined $report) {
> 
> This seems like quite late to find this issue, we should do an early check of all the parameters, before running the tests.

It's not really an issue, though. You don't actually need to report.

>> Tools/Scripts/run-javascriptcore-tests:868
>> +    if (defined $buildbotMaster and defined $builderName and defined $buildNumber and defined $buildbotWorker) {
> 
> There should be else case as well to log the error. We should validate the arguments in beginning to ensure that any required parameter is not missing. Maybe add a validateUploadParameters/validateParameters method.

I think logging something is a good idea, but what's weird about this code is that these options are not mandatory. If you don't provide them, you won't be able to link your test run to a CI link, but that's something the results database allows.

>> Tools/Scripts/run-javascriptcore-tests:876
>> +    if (defined $ENV{'RESULTS_SERVER_API_KEY'}) {
> 
> I believe API key is mandatory for uploading the data. We should check it in beginning and fail early with proper error message if this value is not found.  Maybe add a validateUploadParameters/validateParameters method.

It is not mandatory, actually. Internal results database uses a different authentication scheme.

>> Tools/Scripts/run-javascriptcore-tests:884
>> +    print HANDLE "$encodedUpload\n\0";
> 
> How does the output look like?

This is going to be json, conforming to https://results.webkit.org/documentation#API-Uploads.

>> Tools/Scripts/run-javascriptcore-tests:892
>> +    return 1;
> 
> Can we have another parameter to this script (like --silent-upload-failure / --ignore-upload-failure) which would prevent upload failure from being a hard failure. i.e.: even if upload fails don't mark the script as failed. It would be useful for EWS since the upload can fail due to various reasons (e.g. network issue), and we don't want to have any impact of that failure on patch being processed in EWS.

We're not actually failing when uploads fail, at the moment.

Even so, the --report argument is optional, if it's not provided, we return a successful error code on line 845, not sure that the parameter is acutally useful for EWS....if you don't pass --report, most of this is a no-op

[Aakash Jain]

Seems like tests are not even being run after this change. See https://ews-build.webkit-uat.org/#/builders/21/builds/538

Similarly in old EWS https://webkit-queues.webkit.org/patch/383471/jsc-ews, tests finished within a minute.

[Jonathan Bedard]

Created attachment 383555 [details]
Patch

[Jonathan Bedard]

(In reply to Aakash Jain from comment #14)
> Seems like tests are not even being run after this change. See
> https://ews-build.webkit-uat.org/#/builders/21/builds/538
> 
> Similarly in old EWS https://webkit-queues.webkit.org/patch/383471/jsc-ews,
> tests finished within a minute.

That would be because I uploaded code which comments on the stress tests...clearly not something we should land. I've uploaded a new patch.

[Aakash Jain]

(In reply to Jonathan Bedard from comment #13)
> not sure that the parameter is acutally useful for EWS....if you don't pass --report, most of this is a no-op
for EWS, my intention was to enable test reporting for clean-tree, but I didn't want failure to upload as a hard failure. Anyways, you mentioned that 'We're not actually failing when uploads fail, at the moment'.

[Aakash Jain]

Comment on attachment 383555 [details]
Patch

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

> Tools/Scripts/run-javascriptcore-tests:517
> +    open(TEST, "-|", "@command 2>&1") or $testResult = 1;

This is the part I am little worried about, probably because I don't understand it properly, and it has potential to break the tests completely. Is this well-tested?

> Tools/Scripts/run-javascriptcore-tests:850
> +        print "No report URL specified, skipping results upload\n";

As a user, this seems like an error. Maybe re-phrase something like: 'Skipping upload to results database since no report URL was specified'

> Tools/Scripts/run-javascriptcore-tests:890
> +    open(HANDLE, "|-", "curl -X POST $report/api/upload -H 'Content-Type: application/json' -f -d '\@-'") or die "Failed to open curl";

does Windows/cygwin have curl by default? Also might be a good idea to print the url is error message to make it little more actionable (for e.g.: if the host is down, anyone can verify that)

> Tools/Scripts/run-javascriptcore-tests:901
> +    print "Upload to $report failed\n";

can we print more details about the failure? specific network exception/error. (e.g.: timeout/incorrect-host etc.)

[Jonathan Bedard]

Created attachment 383573 [details]
Patch

[Jonathan Bedard]

(In reply to Aakash Jain from comment #18)
> Comment on attachment 383555 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383555&action=review
> 
> > Tools/Scripts/run-javascriptcore-tests:517
> > +    open(TEST, "-|", "@command 2>&1") or $testResult = 1;
> 
> This is the part I am little worried about, probably because I don't
> understand it properly, and it has potential to break the tests completely.
> Is this well-tested?

I didn't test failing jsc tests originally, but now I have. It's also the exit code bit that's most important for not breaking tests.

> 
> > Tools/Scripts/run-javascriptcore-tests:850
> > +        print "No report URL specified, skipping results upload\n";
> 
> As a user, this seems like an error. Maybe re-phrase something like:
> 'Skipping upload to results database since no report URL was specified'
> 
> > Tools/Scripts/run-javascriptcore-tests:890
> > +    open(HANDLE, "|-", "curl -X POST $report/api/upload -H 'Content-Type: application/json' -f -d '\@-'") or die "Failed to open curl";
> 
> does Windows/cygwin have curl by default? Also might be a good idea to print
> the url is error message to make it little more actionable (for e.g.: if the
> host is down, anyone can verify that)

Not positive, although I believe it does. I'm not sure how we would hit the 'die' here. This isn't the codepath that triggers if curl has a non zero exit code, that's "Upload to ... failed"

> 
> > Tools/Scripts/run-javascriptcore-tests:901
> > +    print "Upload to $report failed\n";
> 
> can we print more details about the failure? specific network
> exception/error. (e.g.: timeout/incorrect-host etc.)

We aren't capturing stdout and stderr, so curl will be printing more details about the failure.

[EWS Watchlist]

Comment on attachment 383573 [details]
Patch

Attachment 383573 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13256291

New failing tests:
apiTests

[Jonathan Bedard]

Created attachment 383587 [details]
Patch

[Aakash Jain]

Comment on attachment 383587 [details]
Patch

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

> Tools/Scripts/run-javascriptcore-tests:904
> +        $upload{'api_key'} = $ENV{'RESULTS_SERVER_API_KEY'};

How does the error looks like when API key is incorrect or missing. Is it easy to debug from the error message?

[Jonathan Bedard]

(In reply to Aakash Jain from comment #23)
> Comment on attachment 383587 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383587&action=review
> 
> > Tools/Scripts/run-javascriptcore-tests:904
> > +        $upload{'api_key'} = $ENV{'RESULTS_SERVER_API_KEY'};
> 
> How does the error looks like when API key is incorrect or missing. Is it
> easy to debug from the error message?

Your error is actually going to come from the results database, which will give you a 401 and 'User is not authorized to POST data'.

[Jonathan Bedard]

Comment on attachment 383587 [details]
Patch

Truitt confirmed that our Cygwin bot do have curl.

[WebKit Commit Bot]

Comment on attachment 383587 [details]
Patch

Clearing flags on attachment: 383587

Committed r252490: <https://trac.webkit.org/changeset/252490>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Paulo Matos]

It seems to me that this patch has broken 32bit JSConly bots.
We are getting right at the beginning "Cannot determine platform".

[Aakash Jain]

(In reply to Paulo Matos from comment #28)
> It seems to me that this patch has broken 32bit JSConly bots.
> We are getting right at the beginning "Cannot determine platform".
Yes, e.g.: https://build.webkit.org/builders/JSCOnly%20Linux%20ARMv7%20Thumb2%20Release/builds/10114/steps/jscore-test/logs/stdio

[Aakash Jain]

(In reply to Paulo Matos from comment #28)
> It seems to me that this patch has broken 32bit JSConly bots.
> We are getting right at the beginning "Cannot determine platform".
Tracked in https://bugs.webkit.org/show_bug.cgi?id=204513

[Jonathan Bedard]

(In reply to Paulo Matos from comment #28)
> It seems to me that this patch has broken 32bit JSConly bots.
> We are getting right at the beginning "Cannot determine platform".

This won't be a hard fix, but someone who has access to one of these machines sort of needs to write it, I don't know what the right platform check would be.

This is all in run-javascriptcore-tests, configurationForUpload.