tests: add artifact snapshot testing #8190
Conversation
|
would these tests be useful to run in LR via smokerider? if yes, it'll be tricky to run these there. if no, awesome nvm :) |
Somewhat, but I'm not terribly concerned about the LR differences here. The primary value of these IMO is flagging when we accidentally break the primary gatherers without realizing we're making a breaking change :) |
There was a problem hiding this comment.
I've been wondering for a while now if we should maybe just replace the whole assertion part of smokehouse with jest assertions (though I was imagining toMatchObject, not snapshots :). I'm nt sure how smokerider complicates making that switch, though.
In that vein, I'm not sure how this realllllllly differs from #8044. We could have smokehouse save artifacts, as its doing now, and call out to this script for verification instead of asserting internally. That would get us all the artifacts we'd ever want to assert in a multitude of testing scenarios (as many as we have smoke tests we can come up with) (though I realize that lessens the ability to specifically keep an eye out on this idea of core (or whatever) artifacts :)
| [path => path[0] === 'content', value => value && value.slice(0, 100)], | ||
| [(_, value) => /localhost:\d+/.test(value), value => value.replace(/localhost:\d+/, 'localhost')], | ||
| [(_, value) => /^blob:/.test(value), () => '<blob url>'], | ||
| ]; |
There was a problem hiding this comment.
can you add a comment (or comments) for each of these
There was a problem hiding this comment.
sure thing if we want to proceed!
| }); | ||
| } else if (typeof value === 'object' && value !== null) { | ||
| Object.entries(value).forEach(([childKey, childValue]) => { | ||
| value[childKey] = cleanArtifactsForSnapshot(childValue, [childKey, ...pathToValue]); |
There was a problem hiding this comment.
applying this recursively definitely seems like it could get fiddly and hard to maintain over time (at least there are snapshots to make sure you aren't deleting someone else's test). e.g. how do I remove score over here but not for everyone else or whatever.
There was a problem hiding this comment.
how do I remove score over here but not for everyone else or whatever.
for posterity, the answer to this question is that you can check the complete path of the value :)
If we move all our smoke tests over to jest assertions then there is no difference :) I thought of this as a first step toward that overall goal, but if we are ready to make the jump now that works for me! 😍 |
|
@patrickhulce, close in favor of the approach in #8962? |
|
yeah if #8962 is happening 😍 |
well, I like where it's going, and everyone else seemed to like it or not say anything, so... :) |
Summary
In addition to @mattzeunert's PR on smoketesting artifacts (#8044) I wanted to offer another proposal for artifact testing that would hopefully lower the barrier to entry and provide us with more robust assertion capabilities as we begin to expose our artifacts as a public API.
This PR adds a way to snapshot test a subset of our artifacts from within jest. Because it's jest-based we can also make any additional assertions we might want.
The primary limitation here is that we are only loading dobetterweb and not all the different nuances of the smoketest pages, so I think each still has their place. This strikes a different balance with the level of detail in our assertions.
Related Issues/PRs
#8044