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

Fix: Remove cacheResultFile attribute from phpunit.xml #582

Merged
merged 1 commit into from
Dec 8, 2018
Merged

Fix: Remove cacheResultFile attribute from phpunit.xml #582

merged 1 commit into from
Dec 8, 2018

Conversation

localheinz
Copy link
Member

This PR:

  • removes the cacheResultFile attribute from phpunit.xml

Fixes #572.

@localheinz localheinz force-pushed the fix/cache-result-file branch 2 times, most recently from 9de9688 to f7c997a Compare December 5, 2018 10:25
localheinz
localheinz commented Dec 5, 2018
View reviewed changes
src/TestFramework/PhpUnit/Config/Builder/InitialConfigBuilder.php Outdated
@@ -210,6 +211,11 @@ private function addRandomTestsOrderAttributesIfNotSet(string $version, \DOMXPat
}
}

private function removeCacheResultFileAttribute(\DOMXPath $xPath): void
{
$this->removeAttribute('cacheResultFile', $xPath);
Copy link
Member Author

Choose a reason for hiding this comment

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

Tested it in localheinz/phpstan-rules by removing the cacheResultFile attribute from the corresponding phpunit.xml, and running infection works fine.

That is, removing the cacheResultFile attribute suffices.

Let me know if you would prefer to remove more attributes.

sanmai
sanmai reviewed Dec 5, 2018
View reviewed changes
src/TestFramework/PhpUnit/Config/Builder/InitialConfigBuilder.php Outdated
@@ -210,6 +211,11 @@ private function addRandomTestsOrderAttributesIfNotSet(string $version, \DOMXPat
}
}

private function removeCacheResultFileAttribute(\DOMXPath $xPath): void
{
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be part of XmlConfigurationHelper?

Copy link
Member Author

Choose a reason for hiding this comment

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

@sanmai

Thank you - give me a moment!

@localheinz localheinz force-pushed the fix/cache-result-file branch 2 times, most recently from 1aeab13 to 4e785a7 Compare December 5, 2018 19:40
@maks-rafalko
Copy link
Member

maks-rafalko commented Dec 5, 2018 •
edited

Initially, you removed cacheResultFile attribute from the "initial test suite" config file in InitialConfigBuilder, which is correct (according to the failed build https://travis-ci.com/localheinz/phpstan-rules/jobs/162844743#L695-L802).

But now it's inside MutationConfigBuilder - this file builds configuration files for each Mutant, not for initial test run.

I think it should be in both InitialConfigBuilder and in MutationConfigBuilder.

Does it make sense? :)

Also, could you please target to 0.11 branch?

@localheinz localheinz changed the base branch from master to 0.11 December 5, 2018 21:35
@localheinz localheinz force-pushed the fix/cache-result-file branch 2 times, most recently from b42c85e to 7065fc2 Compare December 5, 2018 21:38
sanmai
sanmai reviewed Dec 6, 2018
View reviewed changes
src/TestFramework/PhpUnit/Config/XmlConfigurationHelper.php Outdated
}

/** @var \DOMElement $node */
$node = $xPath->query('/phpunit')[0];
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be slightly simpler, for example see removeExistingPrinters

@localheinz localheinz force-pushed the fix/cache-result-file branch 2 times, most recently from 53d3eb1 to 40bca15 Compare December 6, 2018 13:03
maks-rafalko
maks-rafalko approved these changes Dec 8, 2018
View reviewed changes
Copy link
Member

@maks-rafalko maks-rafalko left a comment

Choose a reason for hiding this comment

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

👍

maks-rafalko
maks-rafalko requested changes Dec 8, 2018
View reviewed changes
Copy link
Member

@maks-rafalko maks-rafalko left a comment

Choose a reason for hiding this comment

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

Hm, noticed the failed build. Could you please take a look @localheinz https://travis-ci.org/infection/infection/jobs/464321590#L583?

@localheinz
Copy link
Member Author

@borNfreee

Forgot to adjust the test case. Pushed now!

maks-rafalko
maks-rafalko approved these changes Dec 8, 2018
View reviewed changes
Copy link
Member

@maks-rafalko maks-rafalko left a comment

Choose a reason for hiding this comment

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

Thank you @localheinz

@maks-rafalko maks-rafalko merged commit c13106c into infection:0.11 Dec 8, 2018
@localheinz localheinz deleted the fix/cache-result-file branch December 8, 2018 08:48
@localheinz
Copy link
Member Author

Thank you, @borNfreee and @sanmai!

@maks-rafalko
Copy link
Member

maks-rafalko commented Dec 8, 2018 •
edited

Released 0.11.3

@localheinz
Copy link
Member Author

@borNfreee @sanmai

Just realized that I now end up with a dirty index because the default cacheResult file setting is used now. Should we disable result caching entirely, perhaps?

@maks-rafalko
Copy link
Member

you mean to set cacheResult="false"? Sounds good to me

@localheinz
Copy link
Member Author

On it!

This was referenced Dec 8, 2018
Merged
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sanmai sanmai sanmai left review comments

@maks-rafalko maks-rafalko maks-rafalko approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@localheinz @maks-rafalko @sanmai