-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Use shortenedRecursiveExport()
for data-providers
#5774
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5774 +/- ##
=========================================
Coverage 90.17% 90.17%
Complexity 6582 6582
=========================================
Files 693 693
Lines 19940 19941 +1
=========================================
+ Hits 17980 17981 +1
Misses 1960 1960 ☔ View full report in Codecov by Sentry. |
@sebastianbergmann does this PR depend on the exporter object PR or can it land independently for 11.x ? |
Not completely exporting data provided by data providers is a BC break and should be opt-in. I am still not sure, though, what the right course of action is here "in the big picture". I hope to find time soon to think about this. |
$testData[] = DataFromDataProvider::from( | ||
$dataSetName, | ||
$exporter->export($testCase->providedData()), | ||
$exporter->shortenedRecursiveExport($providedData), | ||
$testCase->dataSetAsStringWithData(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In our usecase, $testCase->dataSetAsStringWithData()
consumes 99% of a runtime as big data provider arrays are dumper without limit. So I am not sure if this PR itself it really helpful...
The should be some test showing what data are problematic and how this helps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provide a reproducer for your problem, so we can verify the fix or build a different one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the slow provider code - https://github.com/mvorisek/sql-formatter/blob/de3d8d9808f130f7a8734f9e93ec3e216c77003f/tests/TokenizerTest.php#L90-L149.
The only dependency is this simple class - https://github.com/mvorisek/sql-formatter/blob/de3d8d9808f130f7a8734f9e93ec3e216c77003f/src/Token.php.
I just realized we already have a
shortenedRecursiveExport
version of the exporter, we just need to useso instead of sebastianbergmann/exporter#55 we could just do this single line change
tested this change on roave/BetterReflection:
phpunit 11.0.8
this PR:
maybe I did not yet understand that you actually try to make this type of exporter configurable via #5773 ?