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

Run PHPStan for the test suite #1329

Merged
merged 50 commits into from
Jan 8, 2025

Conversation

Bellangelo
Copy link
Contributor

@Bellangelo Bellangelo commented Jan 5, 2025

Change Log

Added

  • Run PHPStan for test suite

Fixed

Changed

Removed

Deprecated

Security


Description

Closes: #1340

Copy link

codecov bot commented Jan 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.58%. Comparing base (5758cef) to head (c8757fb).
Report is 4 commits behind head on 1.x.

Additional details and impacted files
@@           Coverage Diff           @@
##              1.x    #1329   +/-   ##
=======================================
  Coverage   82.58%   82.58%           
=======================================
  Files         641      641           
  Lines       17290    17290           
=======================================
  Hits        14279    14279           
  Misses       3011     3011           
Components Coverage Δ
etl 86.00% <ø> (ø)
cli 85.17% <ø> (ø)
lib-array-dot 94.53% <ø> (ø)
lib-azure-sdk 62.60% <ø> (ø)
lib-doctrine-dbal-bulk 97.36% <ø> (ø)
lib-filesystem 76.24% <ø> (ø)
lib-parquet 84.16% <ø> (ø)
lib-parquet-viewer 82.02% <ø> (ø)
lib-rdsl 87.09% <ø> (ø)
lib-snappy 91.24% <ø> (ø)
bridge-filesystem-async-aws 90.60% <ø> (ø)
bridge-filesystem-azure 89.92% <ø> (ø)
bridge-monolog-http 96.38% <ø> (ø)
symfony-http-foundation 77.10% <ø> (ø)
adapter-chartjs 86.45% <ø> (ø)
adapter-csv 89.49% <ø> (ø)
adapter-doctrine 90.14% <ø> (ø)
adapter-elasticsearch 97.19% <ø> (ø)
adapter-google-sheet 78.04% <ø> (ø)
adapter-http 59.15% <ø> (ø)
adapter-json 92.85% <ø> (ø)
adapter-logger 53.84% <ø> (ø)
adapter-meilisearch 97.75% <ø> (ø)
adapter-parquet 59.88% <ø> (ø)
adapter-text 84.44% <ø> (ø)
adapter-xml 83.15% <ø> (ø)

@Bellangelo
Copy link
Contributor Author

Hi @norberttech, codecov failed for some reason and it points to lib/snappy not being fully covered but I haven't touched this file. Do you have any idea what the bug might be here?

@norberttech
Copy link
Member

don't worry about snappy, I think tests there are a bit fragile.
How hard it would be to fix those issues instead of using baseline? I’m not sure how much I like merging it, that's the reason why in the pr about hardening phpstan generics analysis I fixed around 350~ issues which took me far less than I expected to be honest so maybe it's worth investing some extra time and get rid of baseline?

@norberttech
Copy link
Member

question, why do we need a different phpstan configuration for test suite?

@Bellangelo Bellangelo changed the title Run PHPStan in tests Run PHPStan for the test suite Jan 7, 2025
@Bellangelo
Copy link
Contributor Author

question, why do we need a different phpstan configuration for test suite?

Splitting them apart would allow us to have different quality checks for each case. For example, we could have a lower level in our test suite or skip specific errors. Although now, i have unified the configs in this commit 6f4bf08

@github-actions github-actions bot added size: M and removed size: XL labels Jan 7, 2025
@Bellangelo
Copy link
Contributor Author

Hi @norberttech, we have these ignorred errors in our PHPStan config:

ignoreErrors:
- identifier: argument.type
- identifier: missingType.iterableValue

I think we should ignore them using the baseline. The reason is that these errors are not specific to any files or number of occurrence which means that we can keep adding these errors and PHPStan will happily ignore them.

@norberttech
Copy link
Member

Hi @norberttech, we have these ignorred errors in our PHPStan config:

ignoreErrors:

  • identifier: argument.type
  • identifier: missingType.iterableValue

I think we should ignore them using the baseline. The reason is that these errors are not specific to any files or number of occurrence which means that we can keep adding these errors and PHPStan will happily ignore them.

Based on our conversation at Discord I created an ADR that answers this one #1345

@github-actions github-actions bot added size: S and removed size: M labels Jan 8, 2025
@Bellangelo
Copy link
Contributor Author

@norberttech All errors have been fixed. It took a while but no baseline has been used 🚀

@Bellangelo Bellangelo requested a review from norberttech January 8, 2025 02:38
@norberttech
Copy link
Member

you are THE BEST 💪 will check and merge it tomorrow morning

@norberttech norberttech merged commit 2bb79f5 into flow-php:1.x Jan 8, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Run PHPStan for the test suite
2 participants