In markdown break test names with zero-width space #9999
Closed
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
In the `--markdown` format, which is used to display performance test comparisons on GitHub, break the long test names on the camel case word boundary using zero-width space. This prevents the results table from overflowing badly, especially in mobile view. But the **downside** is that you can no longer simply find the tests in the project by the names copied from comparison results, because they now contain invisible characters. Use of zero-with space required the move of some strings to unicode, otherwise the column justification and formatting broke on multibyte characters in UTF-8. Also updated class declarations and private methods to better follow Python conventions. And used list comprehension where it is clearer than map. Added tests for parts of compare_perf_tests.py and their `lit` invocation to the `validation-test`.
|
@gottesmm @shahmishal Please review. |
|
@swift-ci please smoke benchmark |
|
@shahmishal I suggest we also do full test, because I've added |
|
@palimondo smoke tests run the validation tests. |
|
They do not run the long tests though. |
|
@gottesmm Oh, coll. I didn't know that. So… smoke will run the unit tests the way I added them? |
|
@swift-ci please python lint |
Build comment file:Optimized (O)No Changes (278)
Unoptimized (Onone)Improvement (1)
No Changes (277)
Hardware Overview |
|
It didn't help on mobile: The test names are broken on word boundaries, but the other columns are squished as always. 👎 Any ideas? |
|
@shahmishal BTW, nice work on the Jenkins stitching of Build comment file! Could you still have a look where the |
|
If it is in validation-tests... then yes. It will only run them on 1 platform though. |
|
@palimondo |
|
Excuse my very wide ping, but we need to build some consensus here: I'm on the verge with this change. I was really hoping this would improve the display on mobile… But the GitHub's style makes that impossible to fix when all we have access to is Markdown. It eliminates the overflowing table that was hiding SPEEDUP column on desktop, though. It feels like we would be better of fixing the overflowing table by simply renaming the longest tests. Let's say we put the cap on test name length at 40 characters: I have already used such name compression on the new tests for sequence methods: Maybe it would be better to close this PR, if I could get a permission to take an axe to |
|
The long benchmark names ruin my ability to table-ify the output, independent of Markdown. If benchmarks need verbose descriptions, we should add those. 40 chars is already ridiculous for a benchmark name. |
|
My attempt to solve the problem of overflowing results table by breaking the long test names with zero-width spaces has failed. Pinging @lplarson, @dabrahams, @aschwaighofer, @natecook1000 because you have worked on |
|
This will hopefully be resolved by #20334. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
In the
--markdownformat, which is used to display performance test comparisons on GitHub, break the long test names on the camel case word boundary using zero-width space.This prevents the results table from overflowing badly, especially in mobile view. But the downside is that you can no longer simply find the tests in the project by the names copied from comparison results, because they now contain invisible characters.
Use of zero-with space required the move of some strings to unicode, otherwise the column justification and formatting broke on multibyte characters in UTF-8.
Also updated class declarations and private methods to better follow Python conventions. And used list comprehension where it is clearer than map.
Added tests for parts of
compare_perf_tests.pyand theirlitinvocation to thevalidation-test.