Skip to content

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

Merged
samsonasik merged 10 commits intomainfrom
rename-fixture
Apr 24, 2023
Merged

[Test] Rename fixture and refactor to solve random error#3677
samsonasik merged 10 commits intomainfrom
rename-fixture

Conversation

@samsonasik
Copy link
Copy Markdown
Member

@samsonasik
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

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

@samsonasik samsonasik merged commit 10c36b0 into main Apr 24, 2023
@samsonasik samsonasik deleted the rename-fixture branch April 24, 2023 03:05
// write temp file
FileSystem::write($inputFilePath, $inputFileContents);

if (! is_file($inputFilePath)) {
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Development

Successfully merging this pull request may close these issues.

3 participants