Run tests without optimization #14704
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
Member
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
RobinMalfait
force-pushed
the
robin/ensure_existing_spaces_in_attribute_selector_are_valid
branch
from
9ff2d99 to
5e26aec
Compare
RobinMalfait
force-pushed
the
robin/run_tests_without_optimizing
branch
2 times, most recently
from
639407d to
f08b1e7
Compare
thecrypticace
approved these changes
Oct 17, 2024
| } | ||
|
|
||
| export async function run(candidates: string[]) { | ||
| export async function run(candidates: string[], { optimize = true } = {}) { |
Member
There was a problem hiding this comment.
Not a big fan of having to add an object with optimize to every call site. How do you feel about this?
I’m wondering if we:
- Should make this the default (or is the number of
run()calls that do needoptimize: trueeven larger than this subset? - Should instead add a second
runOptimizedexport instead of adding a boolean variable?
Contributor
There was a problem hiding this comment.
Also not a fan but not enough to really care. I think unoptimized is not the default. Pretty sure more tests require an optimized CSS result than not but I don't have a good idea of what the function should be called.
Member
Author
There was a problem hiding this comment.
Funny thing is, I did use a new method initially (14817cb) but run vs runOptimized or vs build all felt wrong, so did a fallback here because typically you run it without.
You could argue that we just don't run the tests through Lightning CSS (which will make the tests bigger but faster).
RobinMalfait
force-pushed
the
robin/ensure_existing_spaces_in_attribute_selector_are_valid
branch
from
71c24fe to
5120b3b
Compare
RobinMalfait
force-pushed
the
robin/run_tests_without_optimizing
branch
2 times, most recently
from
13dd01e to
8fd45ba
Compare
RobinMalfait
force-pushed
the
robin/ensure_existing_spaces_in_attribute_selector_are_valid
branch
from
6aa4ed3 to
7b0465b
Compare
RobinMalfait
force-pushed
the
robin/run_tests_without_optimizing
branch
from
8fd45ba to
ef7332a
Compare
RobinMalfait
force-pushed
the
robin/ensure_existing_spaces_in_attribute_selector_are_valid
branch
from
5e4504d to
8d48594
Compare
RobinMalfait
force-pushed
the
robin/run_tests_without_optimizing
branch
from
ef7332a to
4055bb9
Compare
Base automatically changed from
robin/ensure_existing_spaces_in_attribute_selector_are_valid
to
next
October 18, 2024 17:51
If we generate something incorrect, then Lightning CSS will throw it away which could result in the fact that we generate something wrong but don't know about it because of Lightning CSS. In a lot of tests we know that nothing should be generated, so running an optimization step is not required.
The optimization happens by default, but for the cases where we know we don't need to optimize, we can pass in a flag.
RobinMalfait
force-pushed
the
robin/run_tests_without_optimizing
branch
from
4055bb9 to
d5599c9
Compare
Member
Author
|
Closing this, if we want this we can re-enable it later. |
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.
This PR improves the tests by not running the optimization step (via Lightning CSS) in places where we know that nothing should be generated.
In fact, if we generate invalid CSS where we expected that nothing was being generated, then Lightning CSS will throw the CSS out. This means that we have a bug somewhere in our code, and we should fix it but we don't know about it.
Bonus points: this improves the performance of the tests a tiny bit.