feat: support updating snapshot tests with codelens/hovering/runnables #18757
Conversation
rustbot
added
the
S-waiting-on-review
|
OK, I’ve fixed it. It turned out to be caused by some settings in my editor... |
roife
force-pushed
the
fix-17812
branch
8 times, most recently
from
272dd87 to
8a0598d
Compare
|
Here's a demo. |
crates/ide/src/runnables.rs
Outdated
| self.find_macro("snapbox", &["assert_data_eq", "file", "str"]) | ||
| } | ||
|
|
||
| fn find_macro(&self, crate_name: &str, paths: &[&str]) -> bool { |
There was a problem hiding this comment.
This seems a bit weird to me the way this works, a better idea would be to just check the macro calls of the corresponding body I think and match them up with the expected macros. That is fetch the BodySourceMap for the corresponding body (we might have that via semantics already here, not sure), and iterate the expansions in there to find the macros. Before doing so we should probably first try to resolve all the known testing macros and cache them to just match up the IDs against another
There was a problem hiding this comment.
I use FindUsage because it allows us to quickly filter out irrelevant macro calls by name. Since we need to search within (potentially large) mod/impl blocks, this approach is more efficient. Traversing BodySourceMap and semantics for filtering may lead to a lot of resolving, which might end up being quite slow.
Resolving the testing macros and caching their IDs is reasonable, but I haven’t figured out where to store them yet. 👀
There was a problem hiding this comment.
Resolving the testing macros and caching their IDs is reasonable, but I haven’t figured out where to store them yet
The caching here is good enough to be done at the start of the runnables computation (that is we don't cache across requests)
I use FindUsage because it allows us to quickly filter out irrelevant macro calls by name.
I see, I was thinking of what if the macro call is hidden behind another macro call, but I think that basicalyl breaks all of those frameworks anyways so little point in trrying to do that. I see your approach then. The way this is done here still feels a bit brittle to me though. Will add another comment regarding that in a bit
roife
force-pushed
the
fix-17812
branch
4 times, most recently
from
c2eea6a to
e21d719
Compare
|
I’ve been struggling to pass the I just realized that I’m using Node 18.20.5 (installed with nvm by default), while the CI is on Node 18.4.0 (See https://github.com/actions/setup-node). The |
|
I think it should be fine for us to bump the default node version on our CI then (separate PR) |
Fix #17812
This PR introduces functionality to update snapshot tests.
It detect dependencies of snapshot testing crates first (similar to how
FamousDefs). If any dependencies are found, it usesFindUsagesto determine whether the fn/impl/mod interacts with snapshot testing APIs.Currently, the supported crates include
expect-test,instaandsnapbox.This PR adds the following configurations:
lens.UpdateTest.Enable(default: true).hover.Actions.UpdateTest.Enable(default: true).runnables.AskBeforeUpdateTest(default: true, client-side).