Skip to content

Support automated tests that need a terminal emulator#13033

Closed
yizhepku wants to merge 17 commits intonushell:mainfrom
yizhepku:alacritty-terminal
Closed

Support automated tests that need a terminal emulator#13033
yizhepku wants to merge 17 commits intonushell:mainfrom
yizhepku:alacritty-terminal

Conversation

@yizhepku
Copy link
Copy Markdown
Contributor

@yizhepku yizhepku commented Jun 2, 2024

This PR adds support for tests that need a terminal emulator. It adds the alacritty_terminal crate as a dev-dependency. The terminal state is kept in memory (no actual rendering takes place) and connected to a Nushell process via a PTY.

Two test cases are added as a demonstration, one for auto-cd and one for PWD-aware command hints.

@yizhepku
Copy link
Copy Markdown
Contributor Author

yizhepku commented Jun 2, 2024

I've tested this on Windows and Linux, but it's not working on MacOS yet. I guess I'll dig up my old Macbook.

EDIT: I'm trying to revive my old Macbook. It's a 2014 model, but I have high hopes.

EDIT: Nevermind, it's not a MacOS issue.

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Jun 2, 2024

Thanks for this. If we can get this green, it has the potential to significantly help with testing things we cannot test today. I'm excited about this PR!

@yizhepku
Copy link
Copy Markdown
Contributor Author

yizhepku commented Jun 3, 2024

Turns out the CI failure had nothing to do with MacOS. The problem was that there is no config.nu in the CI environment, so Nushell was asking if I want to create one with default, expecting me to type Y/N in the terminal. The solution is to always use --no-config-file (or specify both --config and --env-config explicitly).

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Jun 3, 2024

Good catch. Seems like there should be a --ci that loads the default configs or something.

@yizhepku
Copy link
Copy Markdown
Contributor Author

yizhepku commented Jun 3, 2024

Seems like there should be a --ci that loads the default configs or something.

Isn't --no-config-file doing just that?

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Jun 3, 2024

It doesn't technically load a default config. That's why when you run nu -n there is no $env.config entry.

@yizhepku
Copy link
Copy Markdown
Contributor Author

yizhepku commented Jun 3, 2024

In any case, --no-config-file works for this PR.

The test case for PWD-aware command hints actually had to set some configs (PWD-aware command hints only work with the sqlite backend, which you explained to me not long ago). The workaround is to create a temporary config file and set both --config and --env-config to it.

@yizhepku
Copy link
Copy Markdown
Contributor Author

yizhepku commented Jun 3, 2024

I wanted to add a test for system-clipboard integration, but I don't understand how Reedline handles it. In particular, I think Ctrl-Shift-C have the same ANSI escape code as Ctrl-C, so how does Reedline differentiate them over SSH?

@rgwood
Copy link
Copy Markdown
Contributor

rgwood commented Jul 7, 2024

This is very cool!

The std::thread::sleep(Duration::from_millis(500)); is a little unfortunate; I wonder if it would make sense to handle waiting inside pty_with_nushell(). Could do something like poll the pty output until it generates the default Nushell prompt (and time out if it's not seen within a few seconds)?

@yizhepku
Copy link
Copy Markdown
Contributor Author

yizhepku commented Jul 7, 2024

I wonder if it would make sense to handle waiting inside pty_with_nushell().

That sounds good.

Could do something like poll the pty output until it generates the default Nushell prompt (and time out if it's not seen within a few seconds)?

I tried a number of approaches (including polling), and sleeping for 500ms seemed to be the most reliable option. I don't really understand what is causing the issue, though, so take it with a grain of salt. In any case, sleeping for 500ms should have little impact on debugging and CI.

@rgwood
Copy link
Copy Markdown
Contributor

rgwood commented Jul 7, 2024

In any case, sleeping for 500ms should have little impact on debugging and CI.

I'm not sure I agree. We have a lot of tests and parallelism is limited. Waiting for 500ms is not a huge deal in 1 or 2 tests but if we start adding more terminal emulator tests this kind of sleep() call will impact overall test times.

Starting up nu on my machine without a config file takes about 15ms. Sleeping for 500ms will probably make each of these tests at least 10x slower.

@yizhepku
Copy link
Copy Markdown
Contributor Author

yizhepku commented Jul 8, 2024

We have a lot of tests and parallelism is limited. Waiting for 500ms is not a huge deal in 1 or 2 tests but if we start adding more terminal emulator tests this kind of sleep() call will impact overall test times.

Really? When I test locally, my CPU usage is saturated at 100% a lot, so I was not expecting sleep() to cause trouble.

Shouldn't parallelism solve the problem? Individual tests would certainly be a lot slower, but I'm hoping that this wouldn't affact CI test time.

@rgwood
Copy link
Copy Markdown
Contributor

rgwood commented Jul 8, 2024

Tools like cargo nextest (which run tests in parallel) generally limit the numbers of tests that can be running at once (to limit memory consumption, I think).

I often run cargo nextest and notice that a batch of relatively slow tests (usually the plugin ones IIRC) block other tests from starting for a long time.

@sholderbach
Copy link
Copy Markdown
Member

What is needed to make progress on this endeavour? Would love to see those added tests, as long as we do not grind our CI to a halt.

@rgwood
Copy link
Copy Markdown
Contributor

rgwood commented Jul 30, 2024

What is needed to make progress on this endeavour?

Possibly nothing. I think the sleep(500ms) is icky and we can+should do better, but it's not necessarily a blocker if @yizhepku doesn't have time for that.

@yizhepku yizhepku marked this pull request as draft October 20, 2024 09:41
@yizhepku
Copy link
Copy Markdown
Contributor Author

yizhepku commented Oct 21, 2024

Closing in favor of nushell/reedline#847, which implements the same functionality without using std::thread::sleep(). I think Reedline might be a better test ground for this type of changes.

@yizhepku yizhepku closed this Oct 21, 2024
@sholderbach
Copy link
Copy Markdown
Member

There may be some rare other places where we want to mock the user interactions like the input command or explore but if you get this going for reedline we are already a big step ahead.

@yizhepku yizhepku deleted the alacritty-terminal branch October 21, 2024 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants