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

Make exporter configurable #5773

Closed
wants to merge 2 commits into from

Conversation

sebastianbergmann
Copy link
Owner

@sebastianbergmann sebastianbergmann commented Mar 28, 2024

PHPUnit uses SebastianBergmann\Exporter\Exporter from sebastian/exporter to export variables as strings, for instance when generating comparison failure messages or when creating immutable value objects representing tests and their data (from data providers) that are passed through the event system to internal as well as third-party subscribers.

In order to allow for the customization of the textual representation of comparison failures, to improve performance (using SebastianBergmann\Exporter\Exporter is expensive when large data structures are involved), and for other use cases it would be nice if SebastianBergmann\Exporter\Exporter could be swapped for a different implementation.

  • Introduce PHPUnit\Util\Exporter interface
  • Implement PHPUnit\Util\DefaultExporter as the default implementation of PHPUnit\Util\Exporter that simply wraps SebastianBergmann\Exporter\Exporter
  • Implement PHPUnit\Util\ExporterFacade that uses PHPUnit\Util\DefaultExporter by default, but can be configured to use an alternative implementation of PHPUnit\Util\Exporter
  • Implement PHPUnit\Util\ExporterChain for chaining PHPUnit\Util\Exporter implementations
  • Use ExporterFacade::instance()->export() instead of SebastianBergmann\Exporter\Exporter::export()
  • Make alternative implementations of PHPUnit\Util\Exporter configurable via PHPUnit's XML configuration file
  • Explore whether SebastianBergmann\Exporter\Exporter::shortenedRecursiveExport() and SebastianBergmann\Exporter\Exporter::shortenedExport should also be wrapped
  • Explore whether the usage of SebastianBergmann\Exporter\Exporter in sebastian/comparator should also be configurable

@sebastianbergmann sebastianbergmann added type/enhancement A new idea that should be implemented feature/test-runner CLI test runner type/performance Issues related to resource consumption (time and memory) feature/data-provider Data Providers feature/assertion Issues related to assertions and expectations labels Mar 28, 2024
@sebastianbergmann sebastianbergmann linked an issue Mar 28, 2024 that may be closed by this pull request
Copy link

codecov bot commented Mar 28, 2024

Codecov Report

Attention: Patch coverage is 65.38462% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 90.06%. Comparing base (1303379) to head (aeb0007).

Files Patch % Lines
src/Util/Exporter/ExporterChain.php 0.00% 13 Missing ⚠️
src/Util/Exporter/DefaultExporter.php 66.66% 2 Missing ⚠️
src/Util/Exporter/ExporterFacade.php 75.00% 2 Missing ⚠️
src/Framework/Constraint/Constraint.php 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5773      +/-   ##
============================================
- Coverage     90.14%   90.06%   -0.08%     
- Complexity     6585     6598      +13     
============================================
  Files           693      696       +3     
  Lines         19943    19966      +23     
============================================
+ Hits          17977    17983       +6     
- Misses         1966     1983      +17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@staabm
Copy link
Contributor

staabm commented Mar 28, 2024

while I agree that a configurable exporter can have benefit for specific use-cases, I would expect the "default exporter" to be fast enough, so I don't need to structure my unit test arround a "limitation" of phpunit, that objects/arrays in data-providers are not efficient enough.

no offense on this end - just my point of view.

@sebastianbergmann
Copy link
Owner Author

no offense on this end - just my point of view

None taken.

I would expect the "default exporter" to be fast enough

Sure!

As I said before, I am not doing this for performance. sebastianbergmann/exporter#55 is likely a more appropriate solution for performance issues.

@BladeMF
Copy link

BladeMF commented Mar 28, 2024

Isn't it better architecture here to have a Factory where one can add multiple exporters (in a config file, probably? or a bootstrap file?) that are called for each type? E.g.

class ExporterFactory 
{
  public function addExporter(ExporterInterface $exporter) { ... }
  
  public function getExporterForValue(mixed $value) {
    foreach($this->exporters as $exporter) {
      if($exporter->supports($value)) return $exporter;
    }
    return $this->defaultExporter;
  }
}

That way doctrine can create their own exporter, I can make my own, etc.

@sebastianbergmann
Copy link
Owner Author

@BladeMF I wanted to keep this as simple as possible, but will ponder your suggestion.

@BladeMF
Copy link

BladeMF commented Mar 28, 2024

In this implementation I have two questions:

  • How will the use method be called? A configuration value (seems best, no)?
  • Will the custom exporter have access to the defaul timplementation to defer some of the work to?

@sebastianbergmann
Copy link
Owner Author

How will the use method be called? A configuration value (seems best, no)?

That is what I mean by "Make alternative implementation of PHPUnit\Util\Exporter configurable via PHPUnit's XML configuration file" in #5773 (comment).

  • Will the custom exporter have access to the defaul timplementation to defer some of the work to?

Currently not, but I will ponder that.

@BladeMF
Copy link

BladeMF commented Mar 28, 2024

Thanks for looking into it.

@sebastianbergmann
Copy link
Owner Author

sebastianbergmann commented Mar 28, 2024

Will the custom exporter have access to the defaul timplementation to defer some of the work to?

I think the best way would be for custom implementations of PHPUnit\Util\Exporter to create their own instance of SebastianBergmann\Exporter\Exporter when needed.

@sebastianbergmann
Copy link
Owner Author

where one can add multiple exporters

I just created PHPUnit\Util\ExporterChain as an implementation of PHPUnit\Util\Exporter to allow for multiple implementations to be used. The first one to return true for PHPUnit\Util\ExporterChain::handles() will be used. The default implementation of PHPUnit\Util\Exporter will be used if no other implementation handles the value to be exported.

@sebastianbergmann
Copy link
Owner Author

I slept on it and now think that SebastianBergmann\Exporter\Exporter should be changed to allow for customizable (and chainable) handling of objects. I do not think that the exporting of scalar values needs to be customizable and the same goes for the recursive processing of arrays.

@sebastianbergmann sebastianbergmann deleted the issue-5758/make-exporter-configurable branch March 29, 2024 05:44
@staabm
Copy link
Contributor

staabm commented Mar 29, 2024

Agree. I also wonder whether all callsites of ->export should be configurable in the same way..? Do all call-sites work if the export shortens the information ? Or do some callsites exist which would break and always need the full export?

@sebastianbergmann
Copy link
Owner Author

Here is a proof-of-concept change for sebastian/exporter: sebastianbergmann/exporter#56

@BladeMF
Copy link

BladeMF commented Mar 29, 2024

I do not think that the exporting of scalar values needs to be customizable and the same goes for the recursive processing of arrays.

Sounds reasonable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/assertion Issues related to assertions and expectations feature/data-provider Data Providers feature/test-runner CLI test runner type/enhancement A new idea that should be implemented type/performance Issues related to resource consumption (time and memory)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants