From 1187eff8a21ff6938d4d00c2a91232fde2d1a796 Mon Sep 17 00:00:00 2001 From: Daniel Silverstone Date: Sat, 6 Jun 2020 11:59:40 +0100 Subject: feat: Support reporting error when more than one binding matches a step Signed-off-by: Daniel Silverstone --- src/ast.rs | 19 ++++++++++--------- src/bindings.rs | 23 ++++++++++------------- src/error.rs | 7 +++++++ src/matches.rs | 11 +---------- 4 files changed, 28 insertions(+), 32 deletions(-) diff --git a/src/ast.rs b/src/ast.rs index bc3547d..4e1d33b 100644 --- a/src/ast.rs +++ b/src/ast.rs @@ -803,15 +803,16 @@ fn step( } let step = step.unwrap(); - let m = bindings.find(&step); - if m.is_none() { - eprintln!("Could not finding binding for: {}", text); - return ( - error_msg(&format!("Could not find binding for: {}", text)), - defkind, - ); - } - let m = m.unwrap(); + let m = match bindings.find(&step) { + Ok(m) => m, + Err(e) => { + eprintln!("Could not select binding: {:?}", e); + return ( + error_msg(&format!("Could not select binding for: {}", text)), + defkind, + ); + } + }; let mut inlines = Vec::new(); diff --git a/src/bindings.rs b/src/bindings.rs index 4233528..ea35f57 100644 --- a/src/bindings.rs +++ b/src/bindings.rs @@ -294,20 +294,17 @@ impl Bindings { /// Find the binding matching a given scenario step, if there is /// exactly one. - pub fn find(&self, step: &ScenarioStep) -> Option { + pub fn find(&self, step: &ScenarioStep) -> Result { let mut matches = self .bindings() .iter() - .map(|b| b.match_with_step(step)) - .filter(|o| o.is_some()); - if let Some(m) = matches.next() { - if matches.count() == 0 { - m - } else { - None - } - } else { - None + .filter_map(|b| b.match_with_step(step)); + match matches.next() { + None => Err(SubplotError::BindingUnknown(step.to_string())), + Some(matched) => match matches.next() { + None => Ok(matched), + Some(_) => Err(SubplotError::BindingNotUnique(step.to_string())), + }, } } @@ -441,7 +438,7 @@ mod test_bindings { let binding = Binding::new(StepKind::When, r"I am Tomjon", "set_foo", None).unwrap(); let mut bindings = Bindings::new(); bindings.add(&binding); - assert!(bindings.find(&step).is_none()); + assert!(bindings.find(&step).is_err()); } #[test] @@ -451,7 +448,7 @@ mod test_bindings { Binding::new(StepKind::Given, r"I am Tomjon of Lancre", "set_foo", None).unwrap(); let mut bindings = Bindings::new(); bindings.add(&binding); - assert!(bindings.find(&step).is_none()); + assert!(bindings.find(&step).is_err()); } #[test] diff --git a/src/error.rs b/src/error.rs index f988c86..daef85c 100644 --- a/src/error.rs +++ b/src/error.rs @@ -34,6 +34,13 @@ pub enum SubplotError { #[error("do not understand binding: {0}")] BindingUnknown(String), + /// Scenario step matches more than one binding + /// + /// THis may be due to bindings being too general, or having unusual + /// overlaps in their matching + #[error("more than one binding matches: {0}")] + BindingNotUnique(String), + /// A binding in the bindings file doesn't specify a known keyword. #[error("binding doesn't specify known keyword: {0}")] BindingWithoutKnownKeyword(String), diff --git a/src/matches.rs b/src/matches.rs index 27a71e1..2c8f196 100644 --- a/src/matches.rs +++ b/src/matches.rs @@ -1,9 +1,7 @@ use crate::Bindings; use crate::Result; use crate::Scenario; -use crate::ScenarioStep; use crate::StepKind; -use crate::SubplotError; use serde::{Deserialize, Serialize}; @@ -20,7 +18,7 @@ impl MatchedScenario { let steps: Result> = scen .steps() .iter() - .map(|step| find_binding(step, bindings)) + .map(|step| bindings.find(step)) .collect(); Ok(MatchedScenario { title: scen.title().to_string(), @@ -29,13 +27,6 @@ impl MatchedScenario { } } -fn find_binding(step: &ScenarioStep, bindings: &Bindings) -> Result { - match bindings.find(step) { - None => Err(SubplotError::BindingUnknown(step.to_string())), - Some(ms) => Ok(ms), - } -} - /// A matched binding and scenario step, with captured parts. /// /// A MatchedStep is a sequence of partial steps, each representing -- cgit v1.2.1 From 3842e769624a2dc3cca5b77269e3ea04e4491d15 Mon Sep 17 00:00:00 2001 From: Daniel Silverstone Date: Sat, 6 Jun 2020 14:31:48 +0100 Subject: bindings: Change to take ownership of passed bindings Signed-off-by: Daniel Silverstone --- src/bindings.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/bindings.rs b/src/bindings.rs index ea35f57..510584d 100644 --- a/src/bindings.rs +++ b/src/bindings.rs @@ -274,15 +274,15 @@ impl Bindings { } /// Add a binding to the set. - pub fn add(&mut self, binding: &Binding) { - self.bindings.push(binding.clone()); + pub fn add(&mut self, binding: Binding) { + self.bindings.push(binding); } /// Add bindings from a YAML string pub fn add_from_yaml(&mut self, yaml: &str) -> Result<()> { let bindings: Vec = serde_yaml::from_str(yaml)?; for b in bindings { - self.add(&from_hashmap(&b)?); + self.add(from_hashmap(&b)?); } Ok(()) } @@ -395,7 +395,7 @@ mod test_bindings { let binding = Binding::new(StepKind::Given, r"I am (?P\S+)", "set_name", None).unwrap(); let mut bindings = Bindings::new(); - bindings.add(&binding); + bindings.add(binding.clone()); assert_eq!(bindings.bindings(), &[binding]); } @@ -437,7 +437,7 @@ mod test_bindings { let step = ScenarioStep::new(StepKind::Given, "given", "I am Tomjon"); let binding = Binding::new(StepKind::When, r"I am Tomjon", "set_foo", None).unwrap(); let mut bindings = Bindings::new(); - bindings.add(&binding); + bindings.add(binding); assert!(bindings.find(&step).is_err()); } @@ -447,7 +447,7 @@ mod test_bindings { let binding = Binding::new(StepKind::Given, r"I am Tomjon of Lancre", "set_foo", None).unwrap(); let mut bindings = Bindings::new(); - bindings.add(&binding); + bindings.add(binding); assert!(bindings.find(&step).is_err()); } @@ -456,7 +456,7 @@ mod test_bindings { let step = ScenarioStep::new(StepKind::Given, "given", "I am Tomjon"); let binding = Binding::new(StepKind::Given, r"I am Tomjon", "set_name", None).unwrap(); let mut bindings = Bindings::new(); - bindings.add(&binding); + bindings.add(binding); let m = bindings.find(&step).unwrap(); assert_eq!(m.kind(), StepKind::Given); let mut parts = m.parts(); @@ -474,7 +474,7 @@ mod test_bindings { let binding = Binding::new(StepKind::Given, r"I am (?P\S+)", "set_name", None).unwrap(); let mut bindings = Bindings::new(); - bindings.add(&binding); + bindings.add(binding); let m = bindings.find(&step).unwrap(); assert_eq!(m.kind(), StepKind::Given); let mut parts = m.parts(); -- cgit v1.2.1 From bf285b4252064e724741e770cc2822a7a33b90ad Mon Sep 17 00:00:00 2001 From: Daniel Silverstone Date: Sat, 6 Jun 2020 14:32:12 +0100 Subject: bindings: Verify that the correct error is returned from Bindings::find() Signed-off-by: Daniel Silverstone --- src/bindings.rs | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/src/bindings.rs b/src/bindings.rs index 510584d..40ef0b4 100644 --- a/src/bindings.rs +++ b/src/bindings.rs @@ -438,7 +438,10 @@ mod test_bindings { let binding = Binding::new(StepKind::When, r"I am Tomjon", "set_foo", None).unwrap(); let mut bindings = Bindings::new(); bindings.add(binding); - assert!(bindings.find(&step).is_err()); + assert!(match bindings.find(&step) { + Err(SubplotError::BindingUnknown(_)) => true, + _ => false, + }); } #[test] @@ -448,7 +451,30 @@ mod test_bindings { Binding::new(StepKind::Given, r"I am Tomjon of Lancre", "set_foo", None).unwrap(); let mut bindings = Bindings::new(); bindings.add(binding); - assert!(bindings.find(&step).is_err()); + assert!(match bindings.find(&step) { + Err(SubplotError::BindingUnknown(_)) => true, + _ => false, + }); + } + + #[test] + fn two_matching_bindings() { + let step = ScenarioStep::new(StepKind::Given, "given", "I am Tomjon"); + let mut bindings = Bindings::default(); + bindings.add(Binding::new(StepKind::Given, r"I am Tomjon", "set_foo", None).unwrap()); + bindings.add( + Binding::new( + StepKind::Given, + &super::regex_from_simple_pattern(r"I am {name}", false).unwrap(), + "set_foo", + None, + ) + .unwrap(), + ); + assert!(match bindings.find(&step) { + Err(SubplotError::BindingNotUnique(_)) => true, + _ => false, + }); } #[test] -- cgit v1.2.1 From 8f73d4a5a1c48dbf1b2a75a6a01ee37b28381e9e Mon Sep 17 00:00:00 2001 From: Daniel Silverstone Date: Sat, 6 Jun 2020 15:52:59 +0100 Subject: test: Add no-binding and multiple-binding check scenarios Signed-off-by: Daniel Silverstone --- subplot.md | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/subplot.md b/subplot.md index b2cdecf..b99f397 100644 --- a/subplot.md +++ b/subplot.md @@ -1811,6 +1811,66 @@ This is another embedded file, and has the same name in uppercase. ``` ~~~~ +## Steps must match bindings + +Subplot permits the binding author to define arbitrarily complex regular +expressions for binding matches. In order to ensure that associating steps +to bindings is both reliable and tractable, a step must match _exactly one_ +binding. + +```{#badbindings.yaml .file .yaml} +- given: a binding + function: a_binding +- given: a (?:broken)? binding + function: a_broken_binding + regex: true +``` + +### Steps which do not match bindings do not work + +~~~~{#nobinding.md .file .markdown} +--- +title: No bindings available +bindings: +- badbindings.yaml +... +# Broken scenario because step has no binding + +```scenario +given a missing binding +then nothing works +``` +~~~~ + +```scenario +given file nobinding.md +and file badbindings.yaml +when I try to run sp-codegen --run nobinding.md -o test.py +then exit code is non-zero +``` + +### Steps which match more than one binding do not work + +~~~~{#twobindings.md .file .markdown} +--- +title: Two bindings match +bindings: +- badbindings.yaml +... +# Broken scenario because step has two possible bindings + +```scenario +given a binding +``` +~~~~ + +```scenario +given file twobindings.md +and file badbindings.yaml +when I try to run sp-codegen --run twobindings.md -o test.py +then exit code is non-zero +``` + ## Embedded graphs -- cgit v1.2.1