[Windows] Add platform agnostic asserts #461

TomasVotruba · 2021-07-19T16:55:43Z

Merge of https://github.com/rectorphp/rector-src/pull/120/checks with vendor detection fixes

TomasVotruba · 2021-07-19T22:54:52Z

@staabm This is rebased version of #120 with some commits cherry-picked to main.

There 2 things left:

make Rector\CodingStyle\Reflection\VendorLocationDetector::detectFunctionLikeReflection() work on windows, that's the 2 failing tests - I think it returns false positive on Windows
Windows "rules-tests" jobs now runs 13 mins, compared to 5 on Linux - that means the build now takes almost 3-times more time to wait; that's way too much for contributing smootly. Somthing like 6-7 mins might be acceptable

Could you check those? First one has higher priority and would be easier to solve IMO.
Thank you

TomasVotruba · 2021-07-19T22:57:00Z

Btw, as an external contributor you should be able to checkout the PR like this:

gh pr checkout 461

And commit right to it.

I find GH CLI extermely for checking PRs from contributors and work with them quickly.

staabm · 2021-07-20T10:16:28Z

This is rebased version of #120 with some commits cherry-picked to main.

thank you so much.

* Windows "rules-tests" jobs now runs 13 mins, compared to 5 on Linux - that means the build now takes almost 3-times more time to wait; that's way too much for contributing smootly. Somthing like 6-7 mins might be acceptable

maybe we should start with testing on windows only in php8 (until we get it faster)?

* make `Rector\CodingStyle\Reflection\VendorLocationDetector::detectFunctionLikeReflection()` work on windows, that's the 2 failing tests - I think it returns false positive on Windows

I will have a look

TomasVotruba · 2021-07-20T14:14:32Z

thank you so much.

I only moved some bits of code. Thank you for all the effort you're putting into this. Lot of Windows users who are stuck on legacy (IMO more easily than any other OS) will benefits from your contributions 👍

maybe we should start with testing on windows only in php8 (until we get it faster)?
only

We actually test only PHP 8, as that's min version before downgrade.

I will have a look

Thanks 👍

TomasVotruba · 2021-07-21T13:40:01Z

@staabm Btw, I've managed to fix the double import @throws collission here:
#478

Took a bit of refactoring all over the place, but it works now 🤗

staabm · 2021-07-23T12:54:27Z

* Windows "rules-tests" jobs now runs 13 mins, compared to 5 on Linux - that means the build now takes almost 3-times more time to wait; that's way too much for contributing smootly. Somthing like 6-7 mins might be acceptable

blackfire profilling reveals that the most time while running tests is spent on rename'ing files

phpunit/phpunit/phpunit .\packages-tests\

phpunit/phpunit .\tests\

staabm · 2021-07-23T13:02:54Z

* make `Rector\CodingStyle\Reflection\VendorLocationDetector::detectFunctionLikeReflection()` work on windows, that's the 2 failing tests - I think it returns false positive on Windows

should be fixed with the latest commit 04feea9

I can't rebase right now from this workstation, but I hope it should be green now.

staabm · 2021-07-24T07:02:11Z

Exporting times of a phpunit run is described at https://stackoverflow.com/a/6775476

anotherway could be using speedtrap: https://phpmagazine.net/2021/02/speedtrap-detect-slow-phpunit-tests.html

maybe we could also investigate in a parallel testsuite with https://theiconic.tech/how-to-reduce-your-php-tests-execution-time-up-to-85-690f70e28d41

alternatively running blackfire directly on the gh-action: https://blog.blackfire.io/github-actions-support-for-blackfire.html

TomasVotruba · 2021-07-24T11:41:50Z

I think this PR is now mixing to many different topics at once - performance, windows in CI, failing test on Windows etc. That may lead to extreme prolonging and overcomplexity. I'd prefer go the other way around, small changes and merge early.

staabm · 2021-07-24T12:15:20Z

The PR does not yet contain changes regarding performance.

Its all about windows github actions.

The comment maybe is misplaced, but I put it here as an repsonse to your initial request.

Is this PR acceptable without performance improvements?

TomasVotruba · 2021-07-24T12:41:52Z

Is this PR acceptable without performance improvements?

If we don't have to wait longer for the feedback from CI than before, it's ok.
It's not about performance, but rather boy scout rule; keeping the original CI quality at least the same.

staabm · 2021-08-07T07:10:44Z

I think we should drop the ci workflow and merge the changes, as it improves already the contribution flow for windows users.

i dont have time right now to analyze the gh-win perf topic

TomasVotruba · 2021-08-08T16:53:39Z

The time got from 2 mins to 7 mins. That would slow down all PRs feedback by ~350 %. Not a way to go if we want to enjoy contributions:

We can merge it if the time difference is ~ 50 % or without running CI for now.

TomasVotruba · 2021-08-08T18:22:07Z

Thank you ☺️

…ctoryRectorTest

TomasVotruba · 2021-08-16T08:07:29Z

👍

samsonasik · 2021-08-16T08:16:57Z

It seems make error in rector prefixed:

rectorphp/rector@4d052b0

https://github.com/rectorphp/rector/runs/3337580361

Utils\Rector\Tests\Rector\RenameSimpleRector\RenameSimpleRectorTest::test with data set #0 (RectorPrefix20210816\Symplify\SmartFileSystem\SmartFileInfo Object (...))
13
Error: Class 'RectorPrefix20210816\PHPUnit\Framework\Constraint\IsEqual' not found

samsonasik · 2021-08-16T08:17:45Z

packages/Testing/PHPUnit/PlatformAgnosticAssertions.php

+namespace Rector\Testing\PHPUnit;
+
+use Nette\Utils\FileSystem;
+use PHPUnit\Framework\Constraint\IsEqual;


this cause error in prefixed rector

https://github.com/rectorphp/rector/runs/3337580361#step:6:12

I created PR #693 for it.

thx @samsonasik

is this something I could have covered before merging the PR?

it is fixed now, you can test scoping by run sh ./full_build.sh locally

TomasVotruba · 2021-09-28T16:13:12Z

Btw, the PlatformAgnosticAssertions trait is breaking compability when using test with PHPUnit 8 and PHPUnit 9. See rectorphp/rector#6709

I could not find a quick solution, so for now, we're removing it from AbstractRectorTestCase, to allow people writing their own rules.

staabm · 2021-09-28T17:32:14Z

From reading rectorphp/rector#6709 I don't see how this issue relates to the trait.

Could you give more details?

TomasVotruba · 2021-09-28T22:22:30Z

You're right, it's not clear from the description. It was the first next blocker there when the reported issue was fixed.

Here it's better described:
rectorphp/rector-phpunit#21 (comment)

Basically overriding native PHPUnit functions leads to BC breaks in case of their argument types + number changing. That's one of main reasons for composition (own methods) over extension (override).

staabm · 2021-11-29T16:04:45Z

Btw, the PlatformAgnosticAssertions trait is breaking compability when using test with PHPUnit 8 and PHPUnit 9. See rectorphp/rector#6709

hey @TomasVotruba ,

I am just working on a similar windows-support topic in a different project.
after analyzing phpunit a bit deeper I found a feature which allows the use of custom Comparator methods.

with https://github.com/staabm/phpunit-cross-os I utilized this API and added the capability to use regular assertEquals* assertions with phpunit, but in a way that differences in DIRECTORY_SEPARATOR and/or EOL-chars are not taken into account on a per TestCase basis.

if time allows I will see whether this also fits rector, without the problems mentioned in rectorphp/rector#6709

TomasVotruba changed the title ~~platform agnostic asserts~~ [Windows] Add platform agnostic asserts Jul 19, 2021

TomasVotruba force-pushed the platform-agnostic-asserts branch 6 times, most recently from 8b0adb8 to d028d34 Compare July 19, 2021 17:25

TomasVotruba enabled auto-merge (squash) July 19, 2021 17:27

TomasVotruba force-pushed the platform-agnostic-asserts branch 4 times, most recently from ce6e5d4 to 94d3b1d Compare July 19, 2021 21:05

This was referenced Jul 19, 2021

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

Closed

[Testing] Use PHP_OS_FAMILY for isWindows() check #464

Closed

staabm mentioned this pull request Jul 23, 2021

Sync FileCacheStorage with changes from phpstan-src #498

Merged

This was referenced Jul 27, 2021

Added MemoryCacheStorage and use it in ChangedFilesDetectorTest #525

Merged

StaticFixtureSplitter->createTemporaryFileInfo() is slow deprecated-packages/symplify#3437

Closed

staabm force-pushed the platform-agnostic-asserts branch from 04feea9 to 72993e3 Compare August 7, 2021 09:37

staabm enabled auto-merge (squash) August 8, 2021 18:20

staabm force-pushed the platform-agnostic-asserts branch from 76b7591 to d5514e4 Compare August 16, 2021 07:06

clxmstaab and others added 17 commits August 16, 2021 09:06

introduce PlatformAgnosticAssertions which handle cross platform issues

ecac68c

Update packages-tests/FileFormatter/ValueObject/NewLineTest.php

4711149

apply PlatformAgnosticAssertions on MoveValueObjectsToValueObjectDire…

930a62a

…ctoryRectorTest

use PlatformAgnosticAssertions in AbstractTestCase

5803386

Github Actions: run tests on windows-latest and ubuntu-latest

9a9d9aa

added PlatformAgnosticAssertions::assertStringEqualsFile()

de33551

Ensure tests manually tear down props (#1)

b477449

check for windows or linux line ending. \n is contained in both.

0d16daa

support assertFileEquals() normalizing

a9d8c44

add OS to name, to separate

694036f

fixed windows compat in VendorLocationDetector

297859b

ParentClassMethodTypeOverrideGuard: fixed windows-compat

0d09f81

ClassMethodParamVendorLockResolver: windows compat

a2602be

Docs

460aeca

Update PlatformAgnosticAssertions.php

3755a54

Delete tests.yaml

c01d12c

Create tests.yaml

d5514e4

staabm merged commit 6cdfebe into main Aug 16, 2021

staabm deleted the platform-agnostic-asserts branch August 16, 2021 07:08

samsonasik reviewed Aug 16, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Windows] Add platform agnostic asserts #461

[Windows] Add platform agnostic asserts #461

TomasVotruba commented Jul 19, 2021 •

edited

TomasVotruba commented Jul 19, 2021 •

edited

TomasVotruba commented Jul 19, 2021

staabm commented Jul 20, 2021 •

edited

TomasVotruba commented Jul 20, 2021

TomasVotruba commented Jul 21, 2021

staabm commented Jul 23, 2021 •

edited

staabm commented Jul 23, 2021 •

edited

staabm commented Jul 24, 2021 •

edited

TomasVotruba commented Jul 24, 2021

staabm commented Jul 24, 2021

TomasVotruba commented Jul 24, 2021 •

edited

staabm commented Aug 7, 2021

TomasVotruba commented Aug 8, 2021

TomasVotruba commented Aug 8, 2021

TomasVotruba commented Aug 16, 2021

samsonasik commented Aug 16, 2021

samsonasik Aug 16, 2021

samsonasik Aug 16, 2021

staabm Aug 16, 2021

samsonasik Aug 16, 2021

TomasVotruba commented Sep 28, 2021

staabm commented Sep 28, 2021

TomasVotruba commented Sep 28, 2021

staabm commented Nov 29, 2021 •

edited

[Windows] Add platform agnostic asserts #461

[Windows] Add platform agnostic asserts #461

Conversation

TomasVotruba commented Jul 19, 2021 • edited

TomasVotruba commented Jul 19, 2021 • edited

TomasVotruba commented Jul 19, 2021

staabm commented Jul 20, 2021 • edited

TomasVotruba commented Jul 20, 2021

TomasVotruba commented Jul 21, 2021

staabm commented Jul 23, 2021 • edited

staabm commented Jul 23, 2021 • edited

staabm commented Jul 24, 2021 • edited

TomasVotruba commented Jul 24, 2021

staabm commented Jul 24, 2021

TomasVotruba commented Jul 24, 2021 • edited

staabm commented Aug 7, 2021

TomasVotruba commented Aug 8, 2021

TomasVotruba commented Aug 8, 2021

TomasVotruba commented Aug 16, 2021

samsonasik commented Aug 16, 2021

samsonasik Aug 16, 2021

Choose a reason for hiding this comment

samsonasik Aug 16, 2021

Choose a reason for hiding this comment

staabm Aug 16, 2021

Choose a reason for hiding this comment

samsonasik Aug 16, 2021

Choose a reason for hiding this comment

TomasVotruba commented Sep 28, 2021

staabm commented Sep 28, 2021

TomasVotruba commented Sep 28, 2021

staabm commented Nov 29, 2021 • edited

TomasVotruba commented Jul 19, 2021 •

edited

TomasVotruba commented Jul 19, 2021 •

edited

staabm commented Jul 20, 2021 •

edited

staabm commented Jul 23, 2021 •

edited

staabm commented Jul 23, 2021 •

edited

staabm commented Jul 24, 2021 •

edited

TomasVotruba commented Jul 24, 2021 •

edited

staabm commented Nov 29, 2021 •

edited