No-Jira - Implement LighthouseCI #908
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
Hurdles to overcome:
|
wrandall22
force-pushed
the
lighthouse-redone
branch
2 times, most recently
from
99a5d31 to
dd50d81
Compare
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
I think we can leave the flows & map pages for now. |
|
What account login are we using? I created an account for DataDog, I don't know if we want to use the same one or a different one. Whatever the account, it should have at least 100 contacts and tasks to reflect load times for normal users |
|
Right now it is the test account (apps+mpdx@cru.org) |
wrandall22
force-pushed
the
lighthouse-redone
branch
from
63f71cb to
36ab4df
Compare
There was a problem hiding this comment.
This is looking great! Thanks for adding it!
Is there an action that will compare the results from main with the results from the current PR to make it easy to see performance improvements and regressions (kind of like the bundle analyzer action does)? Also, would it be possible to add the core web vitals to the logging in addition to the scores in each of the categories?
|
I haven't found an existing action to do the comparison, I think we'd have to set up Lighhouse server to make that work (comparing current PR results to the server results), which would take some buy in and cost with spinning up a server. I'll look into the core web vitals piece. I'm also seeing how hard it would be to post the logs here as a comment. |
|
The bundle analyzer action does it by storing a GHA artifact whenever it runs on |
|
I'm finding that the console log does not match the report summary. If I had to guess, I'd say the console log is probably the last of the 3 runs and the report is the combined median. |
|
If comparing it to the main doesn't work, maybe we could try adding jest tests like on the below medium post. https://medium.com/front-end-weekly/run-lighthouse-performance-audits-on-every-pull-request-6907645d2005 I know we want to hit the PR preview URL, which probably will not work with this, but I thought it was worth sharing in case something is useful. |
|
The GHA itself can do the assertions, but I don't think we're ready for that yet. I'm playing around with the artifact stuff now. The other thing about hitting the preview URL is that, unless we have a way of waiting on the preview job completing, the performance analyzer will fail the first time on every PR. |
wrandall22
force-pushed
the
lighthouse-redone
branch
from
ed393b1 to
12f92ff
Compare
Bundle SizesCompared against c0034cc Route: No significant changes found Dynamic import: None found. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Remaining issues:
|
Scores
Scores
Scores
Scores
Scores
Scores
Scores
Scores
Scores
Scores
Scores
|
|
After pulling in the latest main, I'm getting these errors in the lighthouse browser console:
|
wrandall22
force-pushed
the
lighthouse-redone
branch
from
818efcd to
6d201c1
Compare
Bundle sizes [mpdx-react]Compared against e75367d No significant changes found |
wrandall22
force-pushed
the
lighthouse-redone
branch
3 times, most recently
from
45832b5 to
5d82678
Compare
|
run lighthouse |
|
Comment-driven won't work until this is merged to main. |
wrandall22
force-pushed
the
lighthouse-redone
branch
from
5d82678 to
7217aff
Compare
lighthouse/lighthouse-auth.mjs
Outdated
| } | ||
|
|
||
| return await new Promise((resolve) => { | ||
| const initialLoadId = setInterval(async () => { |
There was a problem hiding this comment.
This doesn't wait for the initial load, which is great. However, it appears that for retries it will load the page, wait one minute, then check the status, introducing an unnecessary delay.
There was a problem hiding this comment.
My understanding is that, if the first load does not succeed, it will then wait 60 seconds, check the status again (which won't have changed) and then reload the page, and keep doing that loop until it finds it. I guess this will mean that we aren't checking the status again right after the next load, which is the unnecessary delay you mention. Perhaps we should also introduce a limit of retries.
There was a problem hiding this comment.
I think it could be simplified a little, but the logic looks great now!
lighthouse/lighthouse-auth.mjs
Outdated
| if (pageFound(originResponse)) { | ||
| resolve(originResponse); | ||
| clearInterval(initialLoadId); | ||
| } else { |
There was a problem hiding this comment.
This branch isn't needed anymore, right?
There was a problem hiding this comment.
It is needed because otherwise it never re-queries the page.
wrandall22
force-pushed
the
lighthouse-redone
branch
from
3199403 to
c8845e7
Compare
lighthouse/lighthouse-auth.mjs
Outdated
| } | ||
| } | ||
|
|
||
| export async function loadInitialPage(page, origin, timeout = 60_000) { |
There was a problem hiding this comment.
Uggh, this waiting approach of waiting on the status code to not be 404 probably won't work. I just made a PR with a preview environment, and while the first build is building, the response to any page is 200 with the following body:
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8">
<title>Welcome</title>
<style>
body {
background-color: #d5dbdb;
margin: 5em 5em 5em 5em;
font-family: "Amazon Ember", Helvetica, Arial, sans-serif;
}
div {
background-color: #ffffff;
padding: 1em 2em 2em 2em;
}
h1 {
color: #16191f;
}
h3 {
color: #16191f;
}
</style>
</head>
<body>
<div>
<h1>Welcome</h1>
<h3>Your app will appear here once you complete your first deployment.</h3>
<p>
Deployment didn't work? Here are some options:
<ul>
<li>Check out <b><a href="https://docs.aws.amazon.com/amplify/latest/userguide/welcome.html">our docs</a></b></li>
<li>Click the <b>Feedback</b> button in the bottom-left corner of the service page</li>
</ul>
</p>
<p>
Quick tips:
<ul>
<li>Have you checked your build settings? The <b>baseDirectory</b> parameter in the
<b>artifacts</b> step of your YAML file should match your build output directory</li>
<li>Building your app should produce an <b>index.html</b> file. Try building your app
locally and check a file with that name exists in the artifacts base directory.</li>
</ul>
</p>
</div>
</body>
</html>We may have to make this method try again if the page content contains Your app will appear here once you complete your first deployment. and hope Amplify doesn't change that placeholder page's content.
There was a problem hiding this comment.
Or we could cycle wait on something on all of our pages, if such a thing exists.
There was a problem hiding this comment.
Looks like I could look for <div id="__next"> which seems to be consistent.
There was a problem hiding this comment.
I've made a commit to this effect, let me know what you think.
….3.0 to get the fix from yarnpkg/berry#4943
…riables to secrets in order to login)
…pt where it was always going to the same URL for at least the logged outputs, so I fixed that while I was in there.
… url. This involved separating out the auth logic to a separate file that can use ES Modules, which was a bit tricky to figure out.
…vironment comes up with a basic landing page so the prior method will not come back with a 404. Instead, we can check for a common div on all of the MPDx pages and move forward after it is up. Preview environments can take up to 13 minutes to deploy
wrandall22
force-pushed
the
lighthouse-redone
branch
from
a47d8a5 to
5c163d8
Compare
Description
Please explain a bullet-point summary of the changes.
List any PRs that this PR is dependent on and any Jira tickets that this PR is related to.
Checklist: