-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
tests: partial artifact update #8802
Conversation
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.
really great idea! So obvious in retrospect, but this whole time I've been carefully manually diffing things and then copying them over like an automaton :)
finalArtifacts[artifactName] = newArtifacts[artifactName]; | ||
fs.writeFileSync( | ||
artifactPath + '/artifacts.json', | ||
JSON.stringify(finalArtifacts, null, 2) + '\n' |
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.
would it be crazy overkill to use assetSaver.saveArtifacts(finalArtifacts, artifactPath)
for this?
JSON.stringify(artifacts, null, 2)
is the same exact thing we do there, but it's possible that will change or the default file name will change, etc, and we'd get those changes for free here.
Only awkwardness is this would need to set finalArtifacts.traces = {};
and finalArtifacts.devtoolsLogs = {};
to keep saveArtifacts
happy.
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.
would it be crazy overkill to use
assetSaver.saveArtifacts(finalArtifacts, artifactPath)
for this?
oh! We should definitely do this, but we should also do
const oldArtifacts = assetSaver.loadArtifacts(artifactPath);
and
const newArtifacts = assetSaver.loadArtifacts(artifactPath);
That would pull in all the devtoolsLogs and traces into oldArtifacts
, which would then be rewritten to the same files on assetSaver.saveArtifacts(finalArtifacts, artifactPath)
, so no need to manually revert the traces and whatnot.
(and if, for some unknown reason someone wants to update the sample traces, yarn run update:sample-artifacts traces
will do the trick)
All this is untested, but hopefully just works :)
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.
All this is untested, but hopefully just works :)
It does! Thanks, this is much better.
Co-Authored-By: mattzeunert <matt@mostlystatic.com>
Co-Authored-By: mattzeunert <matt@mostlystatic.com>
…rome/lighthouse into partial-artifact-update
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.
added some last suggestions, but LGTM!
// Revert everything except the one artifact | ||
const newArtifacts = await assetSaver.loadArtifacts(artifactPath); | ||
const finalArtifacts = oldArtifacts; | ||
finalArtifacts[artifactName] = newArtifacts[artifactName]; |
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.
do we want to log a warning if artifactName
isn't a key on newArtifacts
or oldArtifacts
(need to check both in case it's adding a new one or removing an old one?)
At worst an unknown key will just have no effect and we don't want to throw because it'll leave things in a bad state, but it might be good to let the user know they spelled something wrong or whatever
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.
Will throw now if it can't find the artifact name.
CONTRIBUTING.md
Outdated
yarn run update:sample-json # update sample LHR based on sample artifacts | ||
``` | ||
|
||
Usually you'll need to revert changes to the `*.devtoolslog.json` and `*.trace.json` files and manually review changes to `artifacts.json` to make sure they are related to your work. |
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.
maybe it's unnecessary now? Or
Usually you'll need to revert changes to the `*.devtoolslog.json` and `*.trace.json` files and manually review changes to `artifacts.json` to make sure they are related to your work. | |
If you do a full artifacts update, usually you'll need to revert changes to the `*.devtoolslog.json` and `*.trace.json` files and manually review changes to `artifacts.json` to make sure they are related to your work. |
I don't have a strong opinion on keeping/wording, what do you think would have been helpful for you?
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.
You have to revert those files even if you just do just a partial update, because we just revert the artifacts file.
what do you think would have been helpful for you?
Just having some docs, and knowing that having lots of changes you need to revert is normal.
Added a note saying that the reverting is only necessary when updating artifacts.
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.
You have to revert those files even if you just do just a partial update, because we just revert the artifacts file.
that's one nice side effect of using loadArtifacts
/saveArtifacts
, they automatically handle collecting and resaving the devtoolsLog and trace files too :)
Co-Authored-By: mattzeunert <matt@mostlystatic.com>
Co-Authored-By: mattzeunert <matt@mostlystatic.com>
…rome/lighthouse into partial-artifact-update
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.
wfm!
Summary
Make sample artifacts easier to update by specifying only one artifact that should be updated, like this:
yarn run update:sample-artifacts ScriptElements