chore: added memory tests for missing packages #4264
Conversation
Branch preview |
Tachometer resultsCurrently, no packages are changed by this PR... |
TarunAdobe
force-pushed
the
chore/add-more-memory-tests
branch
from
27d8873
to
c3d5c93
Compare
Lighthouse scores
What is this?Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on Transfer Size
Request Count
|
| testForMemoryLeaks( | ||
| async () => | ||
| await fixture<ActionButton>(BlackActionButton(BlackActionButton.args)) | ||
| ); |
There was a problem hiding this comment.
Not sure if this is useful or not, but should we signature of this method so it works along the lines of:
| testForMemoryLeaks( | |
| async () => | |
| await fixture<ActionButton>(BlackActionButton(BlackActionButton.args)) | |
| ); | |
| testForMemoryLeaks<ActionButton>(BlackActionButton(BlackActionButton.args)); |
I'm not 100% sure the type is even technically required as we're not testing against its actual API here.
Maybe I'm being overly precious and it's not worrying about, but I'm less excited by the ones blow out to the following by the linter:
testForMemoryLeaks(
async () =>
await fixture<Underlay>(
html`
<sp-underlay></sp-underlay>
`
)
);
With this change, it would technically be the below:
testForMemoryLeaks<Underlay>(html`<sp-underlay></sp-underlay>`);
Or even less is the type is unneeded.
🤔
There was a problem hiding this comment.
I think it does improve the readability and conciseness of the test calls but as you rightfully said since we are testing for memory leaks without the knowledge or manipulation of the component's API, we might not need to add the type here.
There was a problem hiding this comment.
These changes look good, but we need to make sure CI is passing before we can merge.
Separately, we'll need to file some follow up work to get this test into the component generator, but not reason to block progress on that. Can you also work with the three in progress new component PRs to ensure this test is included therein?
Yes sure! Also will update the express team to include these memory tests into their alert banner and breadcrumb PRs |
|
We can merge this, because it is green. However, this causes the Chromium tests to take more the 4 minutes when they were previously taking less and a minute. Do we want to look at other options now or ship this and revisit timing/concurrency/segmentation of tests at a later date? |
Yes there is a need to optimize the build time here! Its taking forever. Concurrency shards the build but delays the build time. Its better to segregate the memory test and check this in a separate PR, If you think this is fine let's merge this! |
|
@Westbrook Can we land this? |
🙇🏼
Westbrook
force-pushed
the
chore/add-more-memory-tests
branch
from
df769ac
to
377e5ca
Compare
Added memory tests for remaining packages.
Objective
fixes #3722
Background
Memory leaks in web applications can lead to performance issues and degraded user experience. By adding memory tests to each package in the component library, we can proactively identify and fix potential memory leaks, ensuring the overall stability and performance of the library.
Related Issue:
Building on top of #4228
How has this been tested?
Screenshots (if appropriate)
Types of changes
Checklist
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against
main.