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

Added regression test for bug 8778 #9857

Merged
merged 19 commits into from
Sep 8, 2023
Merged

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Sep 6, 2023

covers #8778

@staabm staabm marked this pull request as ready for review September 6, 2023 16:06
composer dump
../../phpstan clear-result-cache -vvv -q
../../phpstan analyse -vvv -q
! ../../phpstan analyse -vvv --generate-baseline 2>&1 | grep "Result cache not used because the metadata do not match"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we grep for the debug output and because of the leading ! of this line is the exit code of the last pipe inverted.

means we are getting only a successfull exit code, when the text was not matched

Copy link
Member

Choose a reason for hiding this comment

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

This grep business can break really easily.

What I'd like instead:

  1. Add isResultCacheUsed to AnalysisResult (pass $resultCache->isFullAnalysis() to the constructor)
  2. Create FailWithoutResultCacheErrorFormatter. After it checks that result cache was used, we can just call TableErrorFormatter.
  3. Use this error formatter here in this test instead of grep.

composer dump
../../phpstan clear-result-cache -vvv -q
../../phpstan analyse -vvv --generate-baseline 2>&1
! ../../phpstan analyse -vvv -q | grep "Result cache not used because the metadata do not match"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added another test case, because I realized phpstan is also not using the result cache the other way arround

composer dump
../../phpstan clear-result-cache -vvv -q
../../phpstan analyse -vvv -q
! ../../phpstan analyse -vvv --generate-baseline 2>&1 | grep "Result cache not used because the metadata do not match"
Copy link
Member

Choose a reason for hiding this comment

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

This grep business can break really easily.

What I'd like instead:

  1. Add isResultCacheUsed to AnalysisResult (pass $resultCache->isFullAnalysis() to the constructor)
  2. Create FailWithoutResultCacheErrorFormatter. After it checks that result cache was used, we can just call TableErrorFormatter.
  3. Use this error formatter here in this test instead of grep.

composer dump
../../phpstan clear-result-cache -q
../../phpstan analyse -q
../../phpstan analyse -vvv --generate-baseline --error-format=failWithoutResultCache
Copy link
Member

Choose a reason for hiding this comment

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

I just checked AnalyseCommand and the codepath with --generate-baseline actually isn't influenced by the selected error formatter. So this isn't going to fail.

I really like --error-format=failWithoutResultCache and am looking forward to use it in other E2E tests, but not sure what to do about it here.

Copy link
Contributor Author

@staabm staabm Sep 7, 2023

Choose a reason for hiding this comment

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

I had no better idea either. I went back using the grep way for the --generate-baseline case.

to make it more robust, I am doing a initial run, which does not expect the error message beeing in stdout and the subsequent run with the very same string.

hopefully thats good enough.

Copy link
Contributor Author

@staabm staabm Sep 8, 2023

Choose a reason for hiding this comment

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

Sleeping over it I got a different idea.

What about adding a new cli option, e.g --assert-result-cached or --fail-when-no-result-cache instead of using a error-formatter?


edit: maybe instead we could trigger it via ENV var, so the enduser won't see this testing-only infrastructure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

implemented a ENV var approach in phpstan/phpstan-src#2611

@ondrejmirtes ondrejmirtes merged commit 1837fa9 into phpstan:1.10.x Sep 8, 2023
151 of 157 checks passed
@ondrejmirtes
Copy link
Member

Thank you.

ondrejmirtes pushed a commit that referenced this pull request Sep 8, 2023
@staabm staabm deleted the bug8778 branch September 8, 2023 13:54
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.

None yet

2 participants