Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

introduce PlatformAgnosticAssertions trait which handles cross platform issues - adds windows CI coverage #120

Closed
wants to merge 26 commits into from
Closed

introduce PlatformAgnosticAssertions trait which handles cross platform issues - adds windows CI coverage #120

wants to merge 26 commits into from

Conversation

staabm
Copy link
Contributor

@staabm staabm commented May 28, 2021 •
edited

As discussed in #115 (comment) this PR introduced a way which allows to turn assertSame assertions to ignore PHP_EOL differences.

Since I don't like overriding the very fundatmental behaviour across the entire code-base I figured using a simple Trait could be used in Testclasses as an opt-in to a newline normalized assertSame (later maybe more assertion methods).

Adding a trait is very easy and does not require changes in the inheritance hierarchy of the classes which already exist. existing tests currently extend from PHPUnit\Framework\TestCase, some from Rector\Testing\PHPUnit\AbstractTestCase, and some from Rector\Testing\PHPUnit\AbstractRectorTestCase

closes #115 #117 #99

@staabm
Copy link
Contributor Author

staabm commented May 28, 2021

I went ahead and added the PlatformAgnosticAssertions for 3 test-cases so we can get a better feeling whether this approach will fit for the whole codebase.

@staabm staabm mentioned this pull request May 28, 2021
Closed
@staabm staabm changed the title introduce PlatformAgnosticAssertions which handle cross platform issues introduce PlatformAgnosticAssertions trait which handle cross platform issues May 28, 2021
@staabm staabm changed the title introduce PlatformAgnosticAssertions trait which handle cross platform issues introduce PlatformAgnosticAssertions trait which handles cross platform issues May 28, 2021
@staabm
Copy link
Contributor Author

staabm commented May 29, 2021

before proceeding further #118 and #111 should be merged

@staabm staabm force-pushed the platform-agnostic-asserts branch from 24d14ad to 19cac69 Compare May 29, 2021 07:23
@staabm
Copy link
Contributor Author

staabm commented May 29, 2021

@sabbelasichon I guess these test errors are unrelated to my changes?

There were 2 errors:

1) Ssch\TYPO3Rector\Tests\Rector\v9\v5\RefactorProcessOutputRector\RefactorProcessOutputRectorTest::test with data set #0 (Symplify\SmartFileSystem\SmartFileInfo Object (...))
Error: Attempt to assign property "value" on null

/home/runner/work/rector-src/rector-src/src/Rector/v9/v5/RefactorProcessOutputRector.php:103
/home/runner/work/rector-src/rector-src/src/Rector/v9/v5/RefactorProcessOutputRector.php:64
/home/runner/work/rector-src/rector-src/vendor/rector/rector-src/src/Rector/AbstractRector.php:250
/home/runner/work/rector-src/rector-src/vendor/nikic/php-parser/lib/PhpParser/NodeTraverser.php:123
/home/runner/work/rector-src/rector-src/vendor/nikic/php-parser/lib/PhpParser/NodeTraverser.php:223
/home/runner/work/rector-src/rector-src/vendor/nikic/php-parser/lib/PhpParser/NodeTraverser.php:114
/home/runner/work/rector-src/rector-src/vendor/nikic/php-parser/lib/PhpParser/NodeTraverser.php:223
/home/runner/work/rector-src/rector-src/vendor/nikic/php-parser/lib/PhpParser/NodeTraverser.php:91
/home/runner/work/rector-src/rector-src/vendor/rector/rector-src/src/PhpParser/NodeTraverser/RectorNodeTraverser.php:38
/home/runner/work/rector-src/rector-src/vendor/rector/rector-src/src/Application/FileProcessor.php:38
/home/runner/work/rector-src/rector-src/vendor/rector/rector-src/src/Application/FileProcessor/PhpFileProcessor.php:148
/home/runner/work/rector-src/rector-src/vendor/rector/rector-src/src/Application/FileProcessor/PhpFileProcessor.php:165
/home/runner/work/rector-src/rector-src/vendor/rector/rector-src/src/Application/FileProcessor/PhpFileProcessor.php:149
/home/runner/work/rector-src/rector-src/vendor/rector/rector-src/src/Application/FileProcessor/PhpFileProcessor.php:79
/home/runner/work/rector-src/rector-src/vendor/rector/rector-src/src/Application/ApplicationFileProcessor.php:52
/home/runner/work/rector-src/rector-src/vendor/rector/rector-src/src/Application/ApplicationFileProcessor.php:35
/home/runner/work/rector-src/rector-src/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:143
/home/runner/work/rector-src/rector-src/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:105
/home/runner/work/rector-src/rector-src/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:90
/home/runner/work/rector-src/rector-src/tests/Rector/v9/v5/RefactorProcessOutputRector/RefactorProcessOutputRectorTest.php:18

2) Ssch\TYPO3Rector\Tests\Rector\v9\v5\RefactorProcessOutputRector\RefactorProcessOutputRectorTest::test with data set #1 (Symplify\SmartFileSystem\SmartFileInfo Object (...))
Error: Attempt to assign property "value" on null

/home/runner/work/rector-src/rector-src/src/Rector/v9/v5/RefactorProcessOutputRector.php:103
/home/runner/work/rector-src/rector-src/src/Rector/v9/v5/RefactorProcessOutputRector.php:48
/home/runner/work/rector-src/rector-src/vendor/rector/rector-src/src/Rector/AbstractRector.php:250
/home/runner/work/rector-src/rector-src/vendor/nikic/php-parser/lib/PhpParser/NodeTraverser.php:123
/home/runner/work/rector-src/rector-src/vendor/nikic/php-parser/lib/PhpParser/NodeTraverser.php:223
/home/runner/work/rector-src/rector-src/vendor/nikic/php-parser/lib/PhpParser/NodeTraverser.php:114
/home/runner/work/rector-src/rector-src/vendor/nikic/php-parser/lib/PhpParser/NodeTraverser.php:223
/home/runner/work/rector-src/rector-src/vendor/nikic/php-parser/lib/PhpParser/NodeTraverser.php:91
/home/runner/work/rector-src/rector-src/vendor/rector/rector-src/src/PhpParser/NodeTraverser/RectorNodeTraverser.php:38
/home/runner/work/rector-src/rector-src/vendor/rector/rector-src/src/Application/FileProcessor.php:38
/home/runner/work/rector-src/rector-src/vendor/rector/rector-src/src/Application/FileProcessor/PhpFileProcessor.php:148
/home/runner/work/rector-src/rector-src/vendor/rector/rector-src/src/Application/FileProcessor/PhpFileProcessor.php:165
/home/runner/work/rector-src/rector-src/vendor/rector/rector-src/src/Application/FileProcessor/PhpFileProcessor.php:149
/home/runner/work/rector-src/rector-src/vendor/rector/rector-src/src/Application/FileProcessor/PhpFileProcessor.php:79
/home/runner/work/rector-src/rector-src/vendor/rector/rector-src/src/Application/ApplicationFileProcessor.php:52
/home/runner/work/rector-src/rector-src/vendor/rector/rector-src/src/Application/ApplicationFileProcessor.php:35
/home/runner/work/rector-src/rector-src/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:143
/home/runner/work/rector-src/rector-src/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:105
/home/runner/work/rector-src/rector-src/vendor/rector/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:90
/home/runner/work/rector-src/rector-src/tests/Rector/v9/v5/RefactorProcessOutputRector/RefactorProcessOutputRectorTest.php:18

@staabm staabm force-pushed the platform-agnostic-asserts branch from 10800ee to 2bbfd33 Compare May 29, 2021 07:32
@staabm staabm mentioned this pull request May 31, 2021
Merged
@staabm staabm force-pushed the platform-agnostic-asserts branch from e574e50 to 2205b55 Compare June 3, 2021 17:56
@staabm
Copy link
Contributor Author

staabm commented Jun 3, 2021

anyone any ideas how to debug this 'Out of memory' error in the windows github action 'tests for rules-tests' build?

running the testsuite locally takes ages on my pc (and doesnt run out of memory)?

@mallardduck
Copy link
Contributor

mallardduck commented Jun 4, 2021 •
edited

Seems to be around here:

Is Object On Incomplete Class Rector (Rector\Tests\Php72\Rector\FuncCall\IsObjectOnIncompleteClassRector\IsObjectOnIncompleteClassRector)
 ✔  with data set #0
Out of memory
Error: Process completed with exit code 1.

Will need further debugging to know more tho.

UPDATE: welp it looks like my theory about clearing up data in tearDown does change the results.
It was very sketchy since this test ran for a very long time on the windows worker - but it did complete!
However, check this out compared to linux worker lol...
Windows:

Time: 10:02.388, Memory: 6.60 GB
Linux:
Time: 03:39.080, Memory: 6.63 GB

A very significant time increase there - unsure why tho TBH. But at least now it's failing due to failing tests instead of OOMing issues on windows! I'll send a PR to your branch that this PR is made from @staabm .

@mallardduck
Copy link
Contributor

mallardduck commented Jun 4, 2021 •
edited

The 8 test failures on windows can be reviewed here tho too: https://github.com/rectorphp/rector-src/runs/2746052520?check_suite_focus=true

Update:
And the PR to your branch here: staabm#1
..going to close my PR used for testing the workflows.

@staabm staabm force-pushed the platform-agnostic-asserts branch from f3c1957 to 950ce17 Compare June 5, 2021 19:22
@TomasVotruba
Copy link
Member

TomasVotruba commented Jul 13, 2021 •
edited

Hey, I'm going through old open PRs that should be resolved. Last work seems a month ago and the PR is falling behind current version. The rebase might be very frustrating.

The CI is failing here. Any plans to finish it or can we close it?

@clxmstaab clxmstaab force-pushed the platform-agnostic-asserts branch 2 times, most recently from 15ce6b7 to b242c25 Compare July 16, 2021 07:05
@staabm
Copy link
Contributor Author

staabm commented Jul 16, 2021

rebased and made another set of tests pass.

opened new issues on open problems: rectorphp/rector#6571 rectorphp/rector#6572

@staabm
Copy link
Contributor Author

staabm commented Jul 16, 2021

with 0b63e67 I marked this tests to be skipped on windows. that way we could get forward with the PR here and hopefully get it merged. lets see what else is failling.

this skipped tests can be investigated in the separate linked issues

@staabm staabm mentioned this pull request Jul 16, 2021
Closed
@staabm
Copy link
Contributor Author

staabm commented Jul 19, 2021

@TomasVotruba as far as I can tell the remainig errors are not related to the PR and we are good to go

@staabm staabm marked this pull request as ready for review July 19, 2021 08:19
@staabm staabm changed the title introduce PlatformAgnosticAssertions trait which handles cross platform issues introduce PlatformAgnosticAssertions trait which handles cross platform issues - adds windows CI coverage Jul 19, 2021
staabm and others added 15 commits July 19, 2021 12:27
@staabm staabm mentioned this pull request Jul 19, 2021
Closed
@TomasVotruba
Copy link
Member

Separate problem, just discovered by chance :)

@TomasVotruba
Copy link
Member

You can try to fix the root of the problem or remove the @throws annotations. We don't use them anyway

@staabm
Copy link
Contributor Author

staabm commented Jul 19, 2021

You can try to fix the root of the problem or remove the @throws annotations. We don't use them anyway

which @throws annotation where?

@TomasVotruba
Copy link
Member

All those mentioned in failing CI job

@staabm
Copy link
Contributor Author

staabm commented Jul 19, 2021

All those mentioned in failing CI job

oh my.. so much errors in the CI I didn't even see those related to the class I have created with the PR.

thx for the hint - fixed.

@TomasVotruba
Copy link
Member

TomasVotruba commented Jul 19, 2021 •
edited

Thanks for the fixes! I'm going to that issue now, to avoid it in the future 👍

It seems the CI is still failing... could you rebase on our latest main?


I'll give you access as external contributor, so you don't have to ugprade your fork next time. To make your life sligtly easier :) I think you've earned it.

@staabm
Copy link
Contributor Author

staabm commented Jul 19, 2021

It seems the CI is still failing... could you rebase on our latest main?

the PR is already based on the latest main

I'll give you access as external contributor, so you don't have to ugprade your fork next time. To make your life sligtly easier :) I think you've earned it.

thx <3

@TomasVotruba
Copy link
Member

Ok, I'm checking it locally...


See invitation here: https://github.com/rectorphp/rector-src/invitations

@TomasVotruba TomasVotruba mentioned this pull request Jul 19, 2021
Merged
@TomasVotruba
Copy link
Member

Thank you!

Rebased in #461 and partially merged.
Let's continue there

@staabm staabm deleted the platform-agnostic-asserts branch July 20, 2021 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TomasVotruba TomasVotruba TomasVotruba left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@staabm @mallardduck @TomasVotruba @clxmstaab