Skip to content
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

Fix crash on renaming a floating pane (#1323) #1357

Merged
merged 6 commits into from
Apr 29, 2022

Conversation

raphCode
Copy link
Contributor

No description provided.

@raphCode
Copy link
Contributor Author

Ideally we add some tests to avoid regressions, but to be honest, I hate writing tests and also the huge e2e test functions scared me away.
It looks like some sort of Tcl expect just for terminal UI that updates via ANSI sequences?

@imsnif
Copy link
Member

imsnif commented Apr 28, 2022

Ideally we add some tests to avoid regressions, but to be honest, I hate writing tests and also the huge e2e test functions scared me away. It looks like some sort of Tcl expect just for terminal UI that updates via ANSI sequences?

That would be great! The e2e tests are indeed a bit of a handful (as such tests tend to be), but for this maybe you can add an integration test? https://github.com/zellij-org/zellij/blob/main/zellij-server/src/tab/unit/tab_integration_tests.rs

Wdyt?

@raphCode
Copy link
Contributor Author

raphCode commented Apr 28, 2022

Added some tests for renaming a floating and an embedded pane each.


In case you are interested in my experiences adding tests:
It looked difficult at first, because I did not find a definiton of assert_snapshot!. The insta testing library make things actually much easier and also allows to view the rendered zellij UI. Nice!
Also, the existing test code is a bit hard to read at first because there is a lot of implicit state in Tab and non-obvious dependencies like terminal RawFd Numbers.

Copy link
Member

@imsnif imsnif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaking Tab into like... 17 different modules is totally on my list for the not too far future, I very much agree.

Otherwise about the tests - this is good feedback. Do you have suggestions about what we should change that would have made your experience easier?

This LGTM. Please feel free to merge, and don't forget to update the changelog.

@raphCode
Copy link
Contributor Author

Do you have suggestions about what we should change that would have made your experience easier?

Some high level dev docs or a guide on e.g. how to add a test would be super nice. But I totally understand if this maybe is too much to ask for, many projects do not have this and it can also be a maintenance burden since dev docs must be kept up-to-date on code changes.

In general, when doing something for the first time and having no idea on how to approach it, it costs me quite a bit willpower and motivaton to work my way into it. A guide lowers this barrier because it shows a potential path through the problem space and familiarizes me with any new tools involved.
In fact, https://insta.rs/docs/quickstart/ was enough to get to know insta and become comfortable with its workflow. From there, I worked my way backwards through the tests by reverse engineering them: seeing what each line did change in the snapshot.

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.

2 participants