E2E Tests for Query #585
E2E Tests for Query #585
Conversation
modify extend type signature to accept variable # of extendee tables
| - upwards navigation is rly useful; in this codemod, it'd be a lot easier / more performant to query for each function call (generally), update all those that are migrated accordingly (if we have upwards nav | ||
| we can get the statement by walking up tree from the call node). unfortunately, bc some of the migrations require the whole call statement (i.e local var = call), | ||
| we cannot do this and have to query for statements for those transforms that require the whole statement context, and separately query for calls for those transforms that only need the call node |
| - I'm not sure of a good way around this one, just a general performance concern, it's tricky to vary operations between node world / string world. | ||
| When operating on a node, it is not uncommon to want to re-use parts of that node in your transformed output; currently, this will require: | ||
| - for string transform: parsing the node into a string, and interpolating into your returned replacement | ||
| - for node transform: cloning node so it's mutable, updating accordingly (even more impractical, would never do this, just pointing out you CAN) |
There was a problem hiding this comment.
I think we should be biasing towards string transforms. Most codemods are going to be one-and-done, not multi-pass.
There was a problem hiding this comment.
I agree, writing the string transforms was much nicer for this. I guess my point here is that i'm curious if there's a better way to keep parts of existing nodes than using printer
| end | ||
| end) | ||
| end | ||
| end) |
There was a problem hiding this comment.
You should check out fs.walk if it's available: I think that's going to be more consistent for your use cases.
There was a problem hiding this comment.
Bumping this: I think you want fs.walk here.
This definitely looks like this can be abstracted as well.
There was a problem hiding this comment.
What's the main difference you're envisioning? The logic has to be kinda specific since the test cases are all in a flat dir, i.e:
- we walk the directory with all cases; we find
case1.luau. We need to match this specifically toocase1.luau.result,case1.luau.expect. I'm not sure walking makes this any easier?
I've reformatted to use fs.walk, but lmk if there's a different structure you're looking for.
There was a problem hiding this comment.
You could imagine for a larger golden test you have a lot of different subdirectories with different kinds of tests, and maybe different settings for each test.
tests/std/syntax/e2equery.test.luau
Outdated
| local subcase = v.name | ||
| local subcaseBasePath = path.format(path.join(transformCasesPath, subcase)) | ||
| local initialPath = path.format(path.join(subcaseBasePath, "initial.luau")) | ||
| local expectedPath = path.format(path.join(subcaseBasePath, "expected.luau")) | ||
| local resultPath = path.format(path.join(subcaseBasePath, "result.luau")) | ||
|
|
||
| local initialSrc = fs.readfiletostring(initialPath) | ||
|
|
||
| formattedTransform(initialPath, expectedPath, transformPath) | ||
|
|
||
| local result = fs.readfiletostring(initialPath) | ||
| fs.writestringtofile(resultPath, result) | ||
| fs.writestringtofile(initialPath, initialSrc) | ||
|
|
||
| local expectedResult = fs.readfiletostring(expectedPath) | ||
|
|
||
| assert.eq(result, expectedResult) |
There was a problem hiding this comment.
- We already spoke offline about changing the structure here.
- You should think about what happens if a given test doesn't conform to the expected structure.
There was a problem hiding this comment.
On one hand, we could use git diff via a process, but it feels kinda weird to not use an in-house solution. Maybe write a very basic diffing system that compares the output vs snapshot line-by-line?
…to e2e-query-tests
| end | ||
| end) | ||
| end | ||
| end) |
There was a problem hiding this comment.
Bumping this: I think you want fs.walk here.
This definitely looks like this can be abstracted as well.
hgoldstein
left a comment
hgoldstein
left a comment
There was a problem hiding this comment.
See comments, and I believe we should be using all lower case for the path names, so e2ecases and apisurfacechange instead of e2eCases and apiSurfaceChange respectively (yes I see there's a folder that's currently violating that).
tests/std/syntax/e2equery.test.luau
Outdated
| end) | ||
| end | ||
|
|
||
| local transformsDir = "./tests/std/syntax/e2eCases" |
There was a problem hiding this comment.
This feels a bit manual: is there any way for us to get this programmatically?
There was a problem hiding this comment.
Easiest way I know of is with the debug API but not sure if that's the best way. Do you know of any other solutions?
2 participants
No description provided.