-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Conditional and table formatting support for html #4412
Conversation
tests/PhpSpreadsheetTests/Shared/StringHelperInvalidCharTest.php
Outdated
Show resolved
Hide resolved
tests/PhpSpreadsheetTests/Writer/Html/HtmlConditionalFormattingTest.php
Outdated
Show resolved
Hide resolved
Here is something like what I would like to see for HtmlTableFormatWithConditionalTest. |
I did some profiling and found out that the getHashCode functions in Style/ get called a lot. They make the conditional formatting pretty slow. I managed to cache the hash code so that the execution time dropped by 75%. @oleibman Should I commit that to this PR (I think yes) or make another pull request after this one?
|
4272f4d
to
d542036
Compare
Please make a separate PR for your HashCode improvements. |
|
||
public static function colourScaleProvider(): array | ||
{ | ||
return [ |
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.
This is still an overwhelmingly large number of tests. Can you model it after the others and just test a few "interesting" cells in each row, and only for the data you're looking for (background color and value).
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.
It is the test of different color scale formulas. There has to be quite a few data points to have meaningful results as the style is the only measurable data. They are all interesting. I'd say this is the bare minimum.
|
||
public function testConditionalFormattingRulesHtml(): void | ||
{ | ||
$expectedMatches = [ |
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.
Again, this is just too many tests for me to absorb. Please just choose a representative sample. Maybe a couple from column 1 (all of this column seems to match whatever test it needs), the matching cell from column 2, one or two non-matching cells from column 2, and one or two cells from column 3 (these might be a little trickier because conditional formatting doesn't apply to them). It is not that the tests I'm asking you to remove aren't worthwhile - they most decidedly are. For that reason, I would suggest commenting them out rather than deleting them. I think the "missing" tests are better handled by also creating new samples, so that we can confirm visually that the html matches the original xlsx. I will add a comment to your ticket later on how that might be done.
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.
There is a different code path for each test and they use a different conditional case. They are lightning fast as they only check from the same loaded data. I don't understand the rationale of how removing some of them makes the regression testing better.
Making the xlsx to samples makes sense. But I'd still keep these.
As discussed above, I would like you to use your xlsx files for both unit tests and samples. Here is how you might go about it. |
Thank you for all the extra work. I need to do a final review, but should be able to merge this in the next day or two. |
Thank you for your contribution. A complicated but much needed addition. I shall look forward to seeing what you found out about HashCode. |
This is:
Checklist:
Why this change is needed?
Conditional and table formatting support for html writer were needed to provide better html output for processed files.
Fix #1375.