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.

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

[Test] Rename fixture and refactor to solve random error #3677

Merged
merged 10 commits into from Apr 24, 2023

Conversation

samsonasik
Copy link
Member

@samsonasik
Copy link
Member Author

I refactored RenameForeachValueVariableToMatchMethodCallReturnTypeRector to ensure return node only when variable renamed.

@samsonasik samsonasik changed the title [Test] Rename fixture to solve random error [Test] Rename fixture and refactor to solve random error Apr 24, 2023
@samsonasik samsonasik marked this pull request as draft April 24, 2023 02:29
Comment on lines 128 to 131
if (! is_file($inputFilePath)) {
// give enough time to write process
sleep(3);
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on file not exists, it probably file write and read is overlapped, this is to ensure give enough time to write process.

@samsonasik samsonasik marked this pull request as ready for review April 24, 2023 03:04
@samsonasik
Copy link
Member Author

All checks have passed 🎉 @TomasVotruba let's give it a try ;)

@samsonasik samsonasik merged commit 10c36b0 into main Apr 24, 2023
40 checks passed
@samsonasik samsonasik deleted the rename-fixture branch April 24, 2023 03:05
@@ -125,6 +125,14 @@ protected function doTestFile(string $fixtureFilePath): void
// write temp file
FileSystem::write($inputFilePath, $inputFileContents);

if (! is_file($inputFilePath)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why shouldn't the file exists? Where is it deleted?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably overlapped by tearDown

protected function tearDown(): void
{
// clear temporary file
if (is_string($this->inputFilePath)) {
FileSystem::delete($this->inputFilePath);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be verified by dropping the delete.

Do tests exist which re-use the same fixture ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's temporary file with .php file instead of .php.inc so it needs to be deleted, I am thinking that internal phpunit tearDown may cause the overlap

samsonasik added a commit that referenced this pull request May 8, 2023
* [Test] Rename fixture to solve random error

* [ci-review] Rector Rectify

* [ci-review] Rector Rectify

* also rename fixture on PropertyRenameFactoryTest

* increment name

* reduce fixture length

* refactor

* try give enough time to write to temporary file when FileSystem::write() overlapped

* fix phpstan

* retry write

---------

Co-authored-by: GitHub Action <actions@github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants