-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
prevent native REPL from caching state between sessions #24857
Conversation
@microsoft-github-policy-service agree |
First of all, nice job and thank you for doing this. One thing I want to suggest here: Particularly in regards to:
After we dispose the "old" Python REPL server directly, we should add the new pythonServer into disposables array, vscode-python/src/client/repl/replController.ts Lines 9 to 10 in f12d5bc
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 Great work! |
@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. |
I see my test failed. Looking at this again, I realize I did not do this correctly. simply calling |
Hi @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 Have a look at the tests, and let me know if you don't understanding anything :) Thanks. |
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 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
to only run a specific test suite you desire. For example,
would've allowed you to only run the tests in native repl suite locally. |
@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. |
you're most welcome @hutch3232 :) Thanks for your contribution! |
Resolves: #24359
Definitely warrants scrutiny / input as I've never written typescript before. Solved with trial and error + LLMs.