- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 75
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
test(cli): new cli test suite #564
Conversation
fixed a couple of bugs discovered by the new tests
@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
/// 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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, this solves #565 🎉
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 😅 |
television/config/keybindings.rs
Outdated
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) | ||
} | ||
} | ||
|
There was a problem hiding this comment.
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(...),
}
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 😅
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 😅
ec178f7
to
dc80fba
Compare
Hmmm, not sure why that test is flaky 🤔 Edit: Increasing the sleep time seems to solve the problem... |
dc80fba
to
3cd5b7e
Compare
3cd5b7e
to
c14caf5
Compare
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. |
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:
I'm thinking something more straight to the point like:
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 I think TLDR: |
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. |
… of simple docstrings
Thanks for the PR and the updates 🙏🏻, let's merge this 🎉 |
9a3aa8a
into
alexpasmantier:cable-format-rework
new set of test that cover the overall functionality of the cli --------- Co-authored-by: alexandre pasmantier <[email protected]>
new set of test that cover the overall functionality of the cli --------- Co-authored-by: alexandre pasmantier <[email protected]>
new set of test that cover the overall functionality of the cli --------- Co-authored-by: alexandre pasmantier <[email protected]>
new set of test that cover the overall functionality of the cli