Skip to content

test(cli): new cli test suite #564

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

Merged
merged 13 commits into from
Jun 27, 2025

Conversation

lalvarezt
Copy link
Contributor

new set of test that cover the overall functionality of the cli

fixed a couple of bugs discovered by the new tests
@lalvarezt
Copy link
Contributor Author

@alexpasmantier can you help me with the test issues, seems the bat command is not installed (workflow config I assume) and there's a timing problem with the pty because the preview is not loaded (workflow), locally works just fine

testing revealed gaps in the previous implementation
Comment on lines +354 to +358
/// Show the remote control on startup (only works if feature is enabled).
///
/// This flag works identically in both channel mode and ad-hoc mode.
#[arg(long, default_value = "false", verbatim_doc_comment, conflicts_with_all = ["no_remote", "hide_remote"])]
pub show_remote: bool,
Copy link
Owner

Choose a reason for hiding this comment

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

Nice, this solves #565 🎉

@alexpasmantier
Copy link
Owner

Noticed a couple of flaky tests which I fixed in the last commit.

@lalvarezt
Copy link
Contributor Author

Noticed a couple of flaky tests which I fixed in the last commit.

thanks, those were patches to try to get rid of the pesky workflow issue 😅
I don't have anything else for this one, unless you notice something you want to tweak

Comment on lines 43 to 104
impl<'de> Deserialize<'de> for KeyBindings {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: serde::Deserializer<'de>,
{
use serde::de::{MapAccess, Visitor};
use std::fmt;

struct KeyBindingsVisitor;

impl<'de> Visitor<'de> for KeyBindingsVisitor {
type Value = KeyBindings;

fn expecting(
&self,
formatter: &mut fmt::Formatter,
) -> fmt::Result {
formatter.write_str("a map of action names to key bindings")
}

fn visit_map<A>(self, mut map: A) -> Result<KeyBindings, A::Error>
where
A: MapAccess<'de>,
{
let mut bindings = FxHashMap::default();

while let Some(key) = map.next_key::<String>()? {
let binding = map.next_value::<Binding>()?;

// Handle special toggle feature keys
let action = match key.as_str() {
"toggle_remote_control" => {
Action::ToggleFeature(FeatureFlags::RemoteControl)
}
"toggle_preview" => {
Action::ToggleFeature(FeatureFlags::PreviewPanel)
}
"toggle_help" => {
Action::ToggleFeature(FeatureFlags::HelpPanel)
}
"toggle_status_bar" => {
Action::ToggleFeature(FeatureFlags::StatusBar)
}
_ => {
// Try to deserialize as regular Action enum using TOML value
let action_str = toml::Value::String(key.clone());
Action::deserialize(action_str)
.map_err(serde::de::Error::custom)?
}
};

bindings.insert(action, binding);
}

Ok(KeyBindings(bindings))
}
}

deserializer.deserialize_map(KeyBindingsVisitor)
}
}

Copy link
Owner

Choose a reason for hiding this comment

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

I feel like this shouldn't be done here but should be handled when processing actions instead, e.g. keybindings keeps the same derived deserialize but then when a user hits a key that sends Action::ToggleRemoteControl, it's processed as it if were Action::ToggleFeature(FeatureFlags::RemoteControl) and we can isolate that retro compatibility logic in a dedicated function:

fn handle_action(action: &Action) {
    match action {
        ...
        Action::ToggleRemoteControl | Action::ToggleHelp | ... => handle_legacy_actions(...),
    }
}

Copy link
Owner

Choose a reason for hiding this comment

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

That means we keep the legacy Actions for a while and mark them as deprecated in the code and we can just get rid of them and the associated handling logic when we decide to drop support for them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I played a lot with what you are suggesting and always ended up in a place where I had to add a lot of code to support the legacy options (without the de-serializer I mean).

Since the old syntax (toggle_remote_control, toggle_preview, …) is purely an input-format concern, I thought that we could just catch it early and convert it to the new logic.

Also this shouldn't affect the end-user because their config will remain the same, just the developers 🤔

what do you think? perhaps you have a different idea or see a simple way of doing that doesn't affect performance?

Copy link
Owner

Choose a reason for hiding this comment

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

Ah ok, I see what you mean 👍🏻

Taking a look at the code, I think the feature-based logic went a bit too far and shouldn't be coupled with the Action enum like that. I believe a clear indicator of that is the fact that we now need custom logic to map between the way you express an Action in the config and in the code. I got distracted and thought these toggle_help, toggle_remote_control etc actions were legacy and we were getting rid of them but we actually do need these to express actions in the configuration world and be able to tie keybindings to them.

I'm going to try a slight refactoring. Maybe it'll work or maybe it'll turn out terrible and we can stick with the current state of things 😅

Copy link
Owner

Choose a reason for hiding this comment

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

Ok, just pushed a commit that I believe makes things a little less coupled and a bit clearer. wdyt?

Copy link
Contributor Author

@lalvarezt lalvarezt Jun 26, 2025

Choose a reason for hiding this comment

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

looks good, I'll update the doc I was writing on the features feature to reflect this approach instead.
Inadvertently you removed a check that prevented the toggling of the preview when the remote was open. Just open remote and toggle preview, you'll see it changes but the hints are not updated

EDIT: nevermind, I was the one that introduced the issue 😅. I'll add one test to check for this

Copy link
Owner

Choose a reason for hiding this comment

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

Ah yeah ok, I was reading the diff over and over not being able to find what you were referring to 😅

@alexpasmantier
Copy link
Owner

alexpasmantier commented Jun 26, 2025

Hmmm, not sure why that test is flaky 🤔

Edit: Increasing the sleep time seems to solve the problem...

@alexpasmantier
Copy link
Owner

Ok, looks good, tell me when you're ready to merge this 😊

@lalvarezt
Copy link
Contributor Author

Ok, looks good, tell me when you're ready to merge this 😊

ok, fixed the regression and added a test to catch it in the future.
Added new doc for features, please have a look. Other than that it's ok for me

@alexpasmantier
Copy link
Owner

Added new doc for features, please have a look.

Here's my honest opinion:

I don't know if this was done using AI (if it wasn't you're one hell of a productive writer, congrats 😅) but either way the phrasing really makes it feel like it was ai-generated and I think we should change the style to avoid setting off any potential contributors.

I also think the documentation is too verbose and beats too much around the bush.

Example:

Television Features System
The Television Features System is a UI component management framework that provides dual-state control over interface elements. This document provides an in-depth analysis of the features functionality, its architecture, purpose, and configuration.

Television's Features System enables granular control over UI components through a dual-state paradigm. Each feature can be independently controlled along two dimensions:

Enabled/Disabled: Whether the feature's functionality is available
Visible/Hidden: Whether the feature is displayed in the interface

This design pattern allows for UI management where features can exist in three meaningful states: Active (enabled and visible), Hidden (enabled but not visible), and Disabled (completely inactive).
Key Benefits

- Flexible UI Control: Users can customize their interface without losing functionality
- Runtime Toggleability: Features can be dynamically shown/hidden during application use
- Configuration Persistence: Feature states defined in configuration files persist across sessions
- CLI Override Support: Command-line flags can override configured feature states for individual sessions
- Channel-Specific Defaults: Different channels can have different feature configurations

I'm thinking something more straight to the point like:

Features
Television manages UI component states using a system of features that dictate which components of the application should be shown/hidden/enabled/disabled at any given moment.

which I believe conveys in the end almost as much info for any developer reading this.

I wouldn't insist too much on "how this design pattern is great", listing its benefits etc. and would remove that entirely.

I think the module analysis section should actually live next to the code (as docstrings) and doesn't have a lot of added value here without being able to navigate the code.

The Feature Components and State Management sections have nice visuals which we should keep but I would remove documentation that merely lists the methods and properties available (little added value in this context, and the added value is in the details like whether state manipulation methods always succeed or sometimes fail etc. which adds a lot to the overall maintenance surface of the project and I feel should live inside docstrings directly in the code).

I think Advanced Usage in the table of contents doesn't have any associated content.

TLDR:
If I had to rewrite it, I'd make it much shorter, more straight to the point, would add docstrings if necessary and keep the visual diagrams 👍🏻

@lalvarezt
Copy link
Contributor Author

I see what you mean, and yes, it was AI "embellished" based on an initial draft I made. As a personal preference I tend to like more verbose documentation (even though I don't like to write it), but as you say it might look too much AI.

My idea here was that if you're reading the md files you want prose and diagrams, a system's architectural view if you will, and if you want details you should go to the doc generated from docstrings.

That said, I'll follow your suggestions and rephrase some of the sections to match the style you envision.

lalvarezt and others added 2 commits June 27, 2025 09:58
… of simple docstrings
@alexpasmantier
Copy link
Owner

Thanks for the PR and the updates 🙏🏻, let's merge this 🎉

@alexpasmantier alexpasmantier merged commit 9a3aa8a into alexpasmantier:cable-format-rework Jun 27, 2025
4 checks passed
@lalvarezt lalvarezt deleted the cli-tests branch June 27, 2025 11:36
alexpasmantier added a commit that referenced this pull request Jun 28, 2025
new set of test that cover the overall functionality of the cli

---------

Co-authored-by: alexandre pasmantier <[email protected]>
alexpasmantier added a commit that referenced this pull request Jun 28, 2025
new set of test that cover the overall functionality of the cli

---------

Co-authored-by: alexandre pasmantier <[email protected]>
alexpasmantier added a commit that referenced this pull request Jun 28, 2025
new set of test that cover the overall functionality of the cli

---------

Co-authored-by: alexandre pasmantier <[email protected]>
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.

None yet

2 participants