"Change Signature" doesn't work on a nested function

[SamB]

Describe the bug

When I try to run "Change Signature" on a function nested inside another function, it doesn't really work.

At first I thought it wasn't doing anything, but after I wrote a test for it, I realized it might actually be changing the call sites but not the function itself?

(I'm uncertain because the unit tests aren't run using the actual vscode editor, which I understand is partly because Jest didn't seem to provide the APIs you'd need to write a runner analogous to the simple Mocha runner that the VSCode extension docs and examples suggest you use; it looks like Jest has a lot more layers than Mocha? I wonder if https://github.com/macabeus/jest-environment-vscode-extension would work for you?)

Actually, when I run it live it modifies the head of the outer function ...

How to reproduce

I've actually written a test for this:

abracadabra/src/refactorings/change-signature/change-signature.test.ts

Lines 176 to 190 in 0def578

	description: "of a nested function",
	code: `function outer() {
	function [cursor]add(a, b) {
	return a + b;
	}
	return add(1, 2);
	}`,
	expected: `function outer() {
	function add(b, a) {
	return a + b;
	}
	return add(2, 1);
	}`

I've also submitted that as #838 (currently marked draft because I had no issue to reference and I'm not sure you want to merge a failing test), and you can see the failure there.

Expected behavior

I expected changing the signature of a nested function to adjust both the parameters in the function itself, and the arguments passed in calls to the function; at worst, I expected the refactoring to be unavailable.

Screenshots

If applicable, add screenshots to help explain your problem.

Additional information

• Version of the extension impacted: v7.0.0

🧙‍ Add any other context about the problem here.

[nicoespeon]

Hey @SamB, thanks for reporting and creating a test. This was really helpful to debug!

The test was fine. We are not using the actual VS Code editor in most tests so they can run fast. That jest-environment-vscode-extension looks interesting, but is not necessary IMO. Involving the actual VS Code API for most tests will mechanically make them slow. The trade-off is to use the in-memory editor indeed. It replicates the behavior we depend on.

In theory, the behavior we replicate should be covered with a few contract tests: https://github.com/nicoespeon/abracadabra/blob/main/src/editor/editor-contract-test.ts
These tests DO involve the actual VS Code environment. So they are slow, but hopefully there is only a handful. They confirm that the in-memory editor behaves the same as the actual VS Code editor. And indeed, one of the contract test was wrong (our Selection counts lines starting from 0, not 1, and I missed that when reviewing the code).

Interestingly, it wasn't a problem until we get to nested functions. I fixed it (at least, it passes your test + all the existing ones).

So this has been fixed and everything looks good to me now!

[nicoespeon]

@all-contributors please add SamB for bugs

[allcontributors]

@nicoespeon

I've put up a pull request to add @SamB! 🎉