Assert arg groups

src/build/app/mod.rs

@@ -1494,7 +1494,7 @@ impl<'b> App<'b> {
             assert!(
                 self.args.args.iter().filter(|x| x.id == arg.id).count() < 2,
                 "Argument name must be unique\n\n\t'{}' is already in use",
-                arg.id,
+                arg.name,
             );
 
             // Long conflicts
@@ -1592,22 +1592,33 @@ impl<'b> App<'b> {
             );
         }
 
+        for group in &self.groups {
+            // Name conflicts
+            assert!(
+                self.groups.iter().filter(|x| x.id == group.id).count() < 2,
+                "Argument group name must be unique\n\n\t'{}' is already in use",
+                group.name,
+            );
+
+            // Groups should not have naming conflicts with Args
+            assert!(
+                !self.args.args.iter().any(|x| x.id == group.id),
+                "Argument group name '{}' must not conflict with argument name",
+                group.name,
+            );
+
+            // Args listed inside groups should exist
+            for arg in &group.args {
+                assert!(
+                    self.args.args.iter().any(|x| x.id == *arg),
+                    "Argument group '{}' contains non-existent argument",
+                    group.name,
+                )
+            }
+        }
+
         self._panic_on_missing_help(self.g_settings.is_set(AppSettings::HelpRequired));
 
-        // * Args listed inside groups should exist
-        // * Groups should not have naming conflicts with Args
-
-        // * Will be removed as a part of removing String types
-        // let g = groups!(self).find(|g| {
-        //     g.args
-        //         .iter()
-        //         .any(|arg| !(find!(self, arg).is_some() || groups!(self).any(|g| &g.name == arg)))
-        // });
-        // assert!(
-        //     g.is_none(),
-        //     "The group '{}' contains an arg that doesn't exist or has a naming conflict with a group.",
-        //     g.unwrap().name
-        // );
         true
     }
 

tests/groups.rs

@@ -41,14 +41,9 @@ fn required_group_missing_arg() {
     assert_eq!(err.kind, ErrorKind::MissingRequiredArgument);
 }
 
-// This tests a programmer error and will only succeed with debug_assertions
+#[cfg(debug_assertions)]
-// #[cfg(debug_assertions)]
-// TODO: Enable this
 #[test]
-// This used to provide a nice, programmer-friendly error.
+#[should_panic(expected = "Argument group 'req' contains non-existent argument")]
-// Now the error directs the programmer to file a bug report with clap.
-// #[should_panic(expected = "The group 'req' contains the arg 'flg' that doesn't actually exist.")]
-#[should_panic(expected = "internal error")]
 fn non_existing_arg() {
     let _ = App::new("group")
         .arg("-f, --flag 'some flag'")
@@ -61,6 +56,35 @@ fn non_existing_arg() {
         .try_get_matches_from(vec![""]);
 }
 
+#[cfg(debug_assertions)]
+#[test]
+#[should_panic(expected = "Argument group name must be unique\n\n\t'req' is already in use")]
+fn unique_group_name() {
+    let _ = App::new("group")
+        .arg("-f, --flag 'some flag'")
+        .arg("-c, --color 'some other flag'")
+        .group(ArgGroup::with_name("req").args(&["flag"]).required(true))
+        .group(ArgGroup::with_name("req").args(&["color"]).required(true))
+        .try_get_matches_from(vec![""]);
+}
+
+#[test]
+#[should_panic(expected = "Argument group name '' must not conflict with argument name")]
+fn groups_with_name_of_arg_name() {
+    let _ = App::new("group")
+        .arg(Arg::with_name("a").long("a").group("a"))
+        .try_get_matches_from(vec!["", "--a"]);
+}
+
+#[test]
+#[should_panic(expected = "Argument group name 'a' must not conflict with argument name")]
+fn arg_group_with_name_of_arg_name() {
+    let _ = App::new("group")
+        .arg(Arg::with_name("a").long("a").group("a"))
+        .group(ArgGroup::with_name("a"))
+        .try_get_matches_from(vec!["", "--a"]);
+}
+
 #[test]
 fn group_single_value() {
     let res = App::new("group")

tests/unique_args.rs

@@ -1,9 +1,8 @@
 use clap::{App, Arg};
 
-// This tests a programmer error and will only succeed with debug_assertions
 #[cfg(debug_assertions)]
 #[test]
-#[should_panic(expected = "Argument name must be unique")]
+#[should_panic(expected = "Argument name must be unique\n\n\t'arg1' is already in use")]
 fn unique_arg_names() {
     let _ = App::new("some")
         .args(&[
@@ -13,10 +12,9 @@ fn unique_arg_names() {
         .try_get_matches();
 }
 
-// This tests a programmer error and will only succeed with debug_assertions
 #[cfg(debug_assertions)]
 #[test]
-#[should_panic(expected = "Argument short must be unique")]
+#[should_panic(expected = "Argument short must be unique\n\n\t'-a' is already in use")]
 fn unique_arg_shorts() {
     let _ = App::new("some")
         .args(&[
@@ -26,10 +24,9 @@ fn unique_arg_shorts() {
         .try_get_matches();
 }
 
-// This tests a programmer error and will only succeed with debug_assertions
 #[cfg(debug_assertions)]
 #[test]
-#[should_panic(expected = "Argument long must be unique")]
+#[should_panic(expected = "Argument long must be unique\n\n\t'--long' is already in use")]
 fn unique_arg_longs() {
     let _ = App::new("some")
         .args(&[