Problem/Motivation

The file core/tests/Drupal/Tests/Component/Utility/RectangleTest.php takes a very long time to scan for coding standards. This is because there is a humungous array returned by a method. Considering this is test data maybe we should ignore the file.

On PHP 7.2 scanning the entirety of core: Time: 2 mins, 18.16 secs
Of which scanning this file takes: Time: 41.09 secs

Proposed resolution

I've tried to ignore only the huge array. However this didn't work. The only thing that resulted in a performance improvement is adding // @codingStandardsIgnoreFile to the top of the file but as this is a test class I'm loathe to do that. Perhaps a better way would be to just move the data to a different file.

Remaining tasks

Move data to a .json file so that the large dataset does not need to be coding standards checked.

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#12 2911280-2-12.patch251.86 KBalexpott
#12 7-12-interdiff.txt693 bytesalexpott
#9 2911280-2-9.patch251.75 KBalexpott
#7 2911280-7.patch4.05 KBdawehner
#4 2911280-RectangleTest-extra.interdiff.txt852 bytesandypost
#3 2911280-RectangleTest-array-cs-fix.patch284.31 KBandypost
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Parent issue: #2572699: Fix 'Drupal.ControlStructures.InlineControlStructure' coding standard »
Related issues: +#2572699: Fix 'Drupal.ControlStructures.InlineControlStructure' coding standard
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
FileSize
2911280-RectangleTest-array-cs-fix.patch284.31 KB
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
FileSize
2911280-RectangleTest-extra.interdiff.txt852 bytes

Remains from formatting, I guess they out of scope

pazhyn’s picture

pazhyn CreditAttribution: pazhyn as a volunteer and at AnyforSoft, Drupal Ukraine Community for AnyforSoft commented
Issue tags: +kharkiv2017
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

I had some quick look of what actually happens:

\PHP_CodeSniffer_File::start triggers listeners for each token in the php file.
The array decleration sniff \Squiz_Sniffs_Arrays_ArrayDeclarationSniff registers on the array token (both new and old syntax), so it is called OFTEN (see https://blackfire.io/profiles/ebc1fddd-928c-4688-8d0f-be1dfe832b93/graph...)

The call to $phpcsFile->findPrevious(array(T_OPEN_PARENTHESIS, T_SEMICOLON), ($stackPtr - 1), null, false) then seems to be the costly part. Funny enough if you ignore errors, they are just not added later, but the processing is still done, no matter what.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
FileSize
2911280-7.patch4.05 KB

Here is an interdiff which reduces the time significantly ... Down from 50 to 5 seconds on my machine.

The more () we add the faster this scanning gets

The reason why this works is that the line above: $phpcsFile->findPrevious(array(T_OPEN_PARENTHESIS, T_SEMICOLON), ($stackPtr - 1), null, false) tries to find a parenthesis ... which could take a while when there are a lot of tokens involved. The solution is to give them some parenthesis ...

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

Note: I don't think this is the right way to fix it. Moving the data into a json file and then load it would be indeed a simple solution for the problem.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
2911280-2-9.patch251.75 KB

Patch attached moves the data to a json file.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

Before

Testing Drupal\Tests\Component\Utility\RectangleTest

Time: 3.7 seconds, Memory: 14.00MB

OK (4223 tests, 8444 assertions)

After

\Testing Drupal\Tests\Component\Utility\RectangleTest

Time: 4.92 seconds, Memory: 14.00MB

OK (4223 tests, 8444 assertions)

So the patch has not changed the number of assertions.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Needs review » Reviewed & tested by the community

Boggle.

$ cd core/
$ ../vendor/bin/phpcs -ps tests/Drupal/Tests/Component/Utility/RectangleTest.php 
.

Time: 192ms; Memory: 4Mb

$ git reset --hard
HEAD is now at 210ca8a14c Issue #2912333 by vijaycs85, gilesmc, phenaproxima: Revision log field doesn't appear under 'Revision Information' tab
$ ../vendor/bin/phpcs -ps tests/Drupal/Tests/Component/Utility/RectangleTest.php 
.

Time: 5 mins, 35.99 secs; Memory: 58.01Mb

Patch also applies to 8.4.x and while this is a CS improvement it's also a testing improvement (5 extra minutes per branch test...)

Running #9 on 8.4.x. Please consider adding it there, too.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Issue summary: View changes
FileSize
7-12-interdiff.txt693 bytes
2911280-2-12.patch251.86 KB

Discussed with @eli-t and they pointed out that we should probably add a comment as to why the data is all in a json file.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Reviewed & tested by the community » Needs review

Is there an issue about fixing this in coder/phpcs? Maybe add a @todo if there is.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

I opened up an upstream issue: https://github.com/squizlabs/PHP_CodeSniffer/issues/1681

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

@Mile23 I'm not sure that having an @todo for something to change in PHPCS is necessary - also there's not too much advantage to have the data in the same file as the test so even if this is fixed in PHPCS would we bother moving it back?

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Needs review » Reviewed & tested by the community

Thanks for the upstream issue.

Here's an RTBC because putting the data back isn't that big a deal.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

Storing data providers in an external file is totally recommended in phpunit, see https://phpunit.de/manual/current/en/writing-tests-for-phpunit.html#writ...

tacituseu’s picture

tacituseu CreditAttribution: tacituseu commented

It's also responsible for PHP 5.5 & PostgreSQL 9.1 testbot's timeouts/aborts that started after #2901739: Fix 'Squiz.Arrays.ArrayDeclaration' coding standard went in (see: #2901739-44: Fix 'Squiz.Arrays.ArrayDeclaration' coding standard).

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

  • catch committed 48649a8 on 8.5.x 
    Issue #2911280 by alexpott, andypost, dawehner: RectangleTest.php takes...

  • catch committed 7fb892a on 8.4.x 
    Issue #2911280 by alexpott, andypost, dawehner: RectangleTest.php takes...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.