Add integration tests for export plugin structure methods #20004
+102
−0
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
@kamil-tekiela @williamdes followup on #19996 these are the tests to amplify the test coverage on structure exports. |
kamil-tekiela
suggested changes
Dec 31, 2025
Contributor
kamil-tekiela
left a comment
There was a problem hiding this comment.
Thank you so much for these tests. I really appreciate new tests as this makes future work easier. I checked, and they work, but could be improved.
Before this PR is ready to be merged, I have some changes I'd like you to make. Also, I have some general points about this:
- The name
$exportcoreis not a good variable name. For one, it doesn't respect PMA's naming standard, which requires variables to be in camel case.coreis a term that doesn't appear in the rest of the code or in the class name. - Similarly, the test names could be better.
testExportTableStructureThroughExportCorecould betestExportTableStructureIsCalledThroughExportWhenStructureIsRequestedortestStructureIsExportedThroughExportClassWhenRequested. The name should reflect the intent behind the test if it's more than a simple method test. If the tests were in the ExportTest class, it could just betestStructureIsExportedWhenRequested. And of course, when the test has a better name, the docblock above it is redundant as it should be. - As you can see, all of these tests look very similar. But I don't see the positive test for
ExportSql, which should verify that the settings are still respected. They were moved from Export to ExportSql and we should test that this still works. On the other hand, we only need a single test for all other export plugins that don't have that setting. Perhaps, it would be better to just add two new tests, one in ExportTest and one in ExportSqlTest, one testing that ExportSql still works and the other for an arbitrarily chosen other plugin testing to see if exportStructure is called at all.
Comment on lines
728
to
712
| // Force structureOrData to be StructureAndData so structure export is attempted | ||
| $attrStructureOrData = new ReflectionProperty(ExportHtmlword::class, 'structureOrData'); | ||
| $attrStructureOrData->setValue($exportHtmlword, StructureOrData::Structure); |
Contributor
There was a problem hiding this comment.
This should be set through setExportOptions. See lines 549-552.
Tests verify exportStructure() methods are properly invoked through Export::exportTable() for plugins that support export structure Theses tests ensure that when exporting table structure through the Export class, the specific plugin structure export methods are called with the correct parameters Signed-off-by: efturtle <mexslacker@gmail.com>
- Renamed test methods to testExportTableCallsExportStructureMethod for clarity - Removed excessive mocking and focused on integration testing - Simplified test setup by removing unnecessary database/table mocks - Updated assertions to verify actual export output content - Made tests more maintainable and easier to understand - Consistent test structure across ExportHtmlword, ExportLatex, ExportMediawiki, ExportOdt, and ExportPdf plugins Signed-off-by: Alberto Cuevas <mexslacker@gmail.com>
f3768a8 to
47a09da
Compare
Contributor
Author
|
thanks for the suggestions and response on this one. I made a refactor on the tests with your requested changes and recommendations |
Signed-off-by: Alberto Cuevas <mexslacker@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #20004 +/- ##
============================================
+ Coverage 62.05% 62.14% +0.08%
+ Complexity 16112 16098 -14
============================================
Files 676 676
Lines 59990 59979 -11
============================================
+ Hits 37227 37274 +47
+ Misses 22763 22705 -58
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
kamil-tekiela
suggested changes
Jan 5, 2026
Contributor
kamil-tekiela
left a comment
There was a problem hiding this comment.
And then please run:
composer install
vendor/bin/phpcbf
vendor/bin/psalm --set-baseline=psalm-baseline.xml
vendor/bin/phpstan --generate-baseline
- Use this->object instead of creating new plugin instances - set export options instead of reflection Signed-off-by: Alberto Cuevas <mexslacker@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Tests verify exportStructure() methods are properly invoked through Export::exportTable() for plugins that support export structure
Theses tests ensure that when exporting table structure through the Export class, the specific plugin structure export methods are called with the correct parameters