Skip to content

prevent native REPL from caching state between sessions - #24857

Merged
Anthony Kim (anthonykim1) merged 6 commits into
microsoft:mainfrom
hutch3232:issue24359
Apr 2, 2025
Merged

prevent native REPL from caching state between sessions#24857
Anthony Kim (anthonykim1) merged 6 commits into
microsoft:mainfrom
hutch3232:issue24359

Conversation

@hutch3232

@hutch3232 Paul (hutch3232) commented Mar 1, 2025

Copy link
Copy Markdown

Resolves: #24359

Definitely warrants scrutiny / input as I've never written typescript before. Solved with trial and error + LLMs.

@hutch3232

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

@karthiknadig Karthik Nadig (karthiknadig) added the bug Issue identified by VS Code Team member as probable bug label Mar 5, 2025
@anthonykim1

Anthony Kim (anthonykim1) commented Mar 8, 2025

Copy link
Copy Markdown

First of all, nice job and thank you for doing this.

One thing I want to suggest here:

Particularly in regards to:

                    this.pythonServer.dispose();
                    this.pythonServer = createPythonServer([this.interpreter.path as string], this.cwd);

After we dispose the "old" Python REPL server directly, we should add the new pythonServer into disposables array,
similar to

const server = createPythonServer([interpreterPath], cwd);
disposables.push(server);

when we create Python (REPL) Server on createReplController function.

So essentially after the above referenced code that you contributed (in src/client/repl/nativeRepl.ts) could add line this.disposables.push(this.pythonServer);

Great work!

@hutch3232

Copy link
Copy Markdown
Author

Anthony Kim (@anthonykim1) I saw that I had a CI failure due to not having included a test for the new code. I have attempted to add that now.

Full disclosure: I don't know if the test actually works. I had planned to run it before and after applying my proposed fix, but was unable to. I tried very hard to run the test suite but could not get it to go. Even using the included .devcontainer was giving me a lot of issues.

@hutch3232

Copy link
Copy Markdown
Author

I see my test failed. Looking at this again, I realize I did not do this correctly. simply calling dispose() I don't think is right. I think to correctly do this I would need to simulate workspace.onDidCloseNotebookDocument event. I could use some guidance on that if possible.

@anthonykim1

Anthony Kim (anthonykim1) commented Apr 1, 2025

Copy link
Copy Markdown

Hi Paul (@hutch3232)

I've added some test to test out notebook close, along with tracking pythonServer gets called when notebook close event is called.

The key to this was figuring out triggering onDidCloseNotebook event, which signifies repl editor tab closing. Since we can't explicitly cursor click to close the repl tab during testing, we mock this with
notebookCloseEmitter.fire(closingNotebookDocument);

Have a look at the tests, and let me know if you don't understanding anything :)

Thanks.

@anthonykim1

Anthony Kim (anthonykim1) commented Apr 1, 2025

Copy link
Copy Markdown

FYI to further encourage and enhance future contributions (specific to creating tests and testing them locally), you can click on these different type of run options for tests on your run and debug panel, where you see all these different tests you can run.

It's worth pointing out that when you click on the setting gear icon here, it leads to .json tab where you can edit the

"VSC_PYTHON_CI_TEST_GREP": "" // Modify this to run a subset of the single workspace tests

to only run a specific test suite you desire.

For example,

`"VSC_PYTHON_CI_TEST_GREP": "REPL - Native REPL"

would've allowed you to only run the tests in native repl suite locally.

Screenshot 2025-04-01 at 11 28 43 AM

@anthonykim1
Anthony Kim (anthonykim1) merged commit 3275c34 into microsoft:main Apr 2, 2025
@hutch3232

Copy link
Copy Markdown
Author

Anthony Kim (@anthonykim1) thank you for your help finishing this! I think I would have struggled to figure all of that out. Anyways, I will try to get the tests working on my end for future development purposes.

@anthonykim1

Copy link
Copy Markdown

you're most welcome Paul (@hutch3232) :)
Feel free to directly ping me if you need help in future PRs in the repo.

Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-repl bug Issue identified by VS Code Team member as probable bug skip-issue-check

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Native REPL wrongly caches state from previous session

3 participants