Bug 231374 – [webkitperl] Fix webkitperl on Windows

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Basuke Suzuki]

Some tests are failed because of the handling of environment variables.

[Basuke Suzuki]

Created attachment 440516 [details]
PATCH

[Basuke Suzuki]

Created attachment 440524 [details]
PATCH

[Basuke Suzuki]

Created attachment 440526 [details]
PATCH

[Stephan Szabo]

Comment on attachment 440526 [details]
PATCH

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

> Tools/Scripts/webkitperl/auto-version_unittest/autoVersionTests.pl:703
> +        $ENV{RC_ProjectSourceVersion} = $testCase->{'RC_ProjectSourceVersion'};

I think if you're going to set our ENV like this, you'll need to reset it between tests, otherwise values will leak from test to test (or doing something more complicated that will only set the env in the created shell).

> Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/generateFunctionLists.pl:508
> +    plan skip_all => 'Please add `pathc` to the $PATH';

Nitpick: pathc -> patch

[Fujii Hironori]

Comment on attachment 440526 [details]
PATCH

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

> Tools/Scripts/webkitperl/auto-version_unittest/autoVersionTests.pl:691
> +# Test can only be run if VersionStamper.exe exists

This comment doesn't match with the code. Which do yo want to check VersionStamper.exe or auto-version.pl here?

> Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/generateFunctionLists.pl:34
> +use VCSUtils;

Why do you need this change?

> Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/generateFunctionLists.pl:509
> +    exit 0;    

I think the test should fail if patch isn't found.

[Basuke Suzuki]

Comment on attachment 440526 [details]
PATCH

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

>> Tools/Scripts/webkitperl/auto-version_unittest/autoVersionTests.pl:691
>> +# Test can only be run if VersionStamper.exe exists
> 
> This comment doesn't match with the code. Which do yo want to check VersionStamper.exe or auto-version.pl here?

Correct. Thanks.

>> Tools/Scripts/webkitperl/auto-version_unittest/autoVersionTests.pl:703
>> +        $ENV{RC_ProjectSourceVersion} = $testCase->{'RC_ProjectSourceVersion'};
> 
> I think if you're going to set our ENV like this, you'll need to reset it between tests, otherwise values will leak from test to test (or doing something more complicated that will only set the env in the created shell).

I thought each test will be run in different process, but there seems no such assumption at all. I'll save and restore them at the end of each loop.

>> Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/generateFunctionLists.pl:34
>> +use VCSUtils;
> 
> Why do you need this change?

It was failed to load VCSUtils on the initial location. I bet this is related with the change on 5.26:
https://metacpan.org/pod/perl5260delta#Removal-of-the-current-directory-(%22.%22)-from-@INC

Anyway, it is dependent on the @INC so it is reasonable to move the usage bellow that.

>> Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/generateFunctionLists.pl:508
>> +    plan skip_all => 'Please add `pathc` to the $PATH';
> 
> Nitpick: pathc -> patch

Right.

>> Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/generateFunctionLists.pl:509
>> +    exit 0;    
> 
> I think the test should fail if patch isn't found.

Well, I want to hear the opinion from others.

[Radar WebKit Bug Importer]

<rdar://problem/84262724>

[Basuke Suzuki]

Created attachment 441640 [details]
patch

[Basuke Suzuki]

Make `patch` required to run test.
Also environment variable is recovered after getting result from process invocation.

[EWS]

Committed r284405 (?): <https://commits.webkit.org/r284405>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 441640 [details].