YR/-Convert-IN116-into-BC111/CIAC-10243

demisto_sdk/commands/validate/tests/BC_validators_test.py

@@ -946,57 +946,82 @@ def test_IsValidToversionOnModifiedValidator_is_valid(content_items, old_content
     )
 
 
-def test_HaveCommandsOrArgsNameChangedValidator__script__fails():
+@pytest.mark.parametrize(
+    "new_args, breaking_arg",
+    [
+        pytest.param(
+            [{"name": "arg1", "required": False}, {"name": "arg2", "required": True}],
+            "arg2",
+            id="two args, new one required",
+        ),
+        pytest.param(
+            [
+                {"name": "arg1", "required": True},
+            ],
+            "arg1",
+            id="one arg, required",
+        ),
+    ],
+)
+def test_NewRequiredArgumentScriptValidator__fails(new_args, breaking_arg):
     """
     Given
-        - An old script with 2 non-required args.
-        - A new script with the same first 2 arguments, but they are required, and a new arg added.
+        - An old script with 1 non-required arg.
+        - A new script with a required arg:
+        case 1: the required arg is new.
+        case 2: the old arg was changed to be required.
     When
         calling the NewRequiredArgumentScriptValidator.
     Then
-        - Make sure the validation fails and the right error message is returned.
+        - Make sure the validation fails and the right breaking arg is mentioned in the error message.
     """
     new_content_item = create_script_object(
         paths=["args"],
-        values=[
-            [
-                {"name": "arg1", "required": True},
-                {"name": "arg2", "required": True},
-                {"name": "arg3"},
-            ]
-        ],
-    )
-    old_content_item = create_script_object(
-        paths=["args"], values=[[{"name": "arg1"}, {"name": "arg2"}]]
+        values=[new_args],
     )
+    old_content_item = create_script_object(paths=["args"], values=[[{"name": "arg1"}]])
 
     create_old_file_pointers([new_content_item], [old_content_item])
     res = NewRequiredArgumentScriptValidator().is_valid([new_content_item])
-    assert (
-        res[0].message
-        == 'Possible backward compatibility break: You have added the following new *required* arguments: "arg1", "arg2". Please undo the changes.'
-    )
+    assert breaking_arg in res[0].message
 
 
-def test_HaveCommandsOrArgsNameChangedValidator__script__passes():
+@pytest.mark.parametrize(
+    "new_args",
+    [
+        pytest.param(
+            [
+                {"name": "arg1", "required": False},
+                {"name": "arg2", "required": False},
+            ],
+            id="two args, both not required",
+        ),
+        pytest.param(
+            [
+                {"name": "arg1", "required": True},
+                {"name": "arg2", "required": True, "defaultvalue": "test"},
+            ],
+            id="two args, both required, second one with default value",
+        ),
+    ],
+)
+def test_NewRequiredArgumentScriptValidator__passes(new_args):
     """
     Given
-        - An old script with 2 non-required args.
-        - A new script with the same first 2 arguments, but they are required, and a new arg added.
+        - An old script with 2 args, one required and one not.
+        - A new script:
+        case 1: with 2 args, both not required.
+        case 2: with 2 args, both required, and the second one has a default value.
     When
         calling the NewRequiredArgumentScriptValidator.
     Then
-        - Make sure the validation passes, since the new required arguments have a default value, or are not new.
+        - Make sure the validation passes:
+        case 1: the first arg was changed to be not required which is allowed.
+        case 2: the first arg did not change, and the second arg was changed to be required but has a default value which is allowed.
     """
     new_content_item = create_script_object(
         paths=["args"],
-        values=[
-            [
-                {"name": "arg1", "required": True},
-                {"name": "arg2", "required": True, "defaultvalue": "test"},
-                {"name": "arg3"},
-            ]
-        ],
+        values=[new_args],
     )
     old_content_item = create_script_object(
         paths=["args"], values=[[{"name": "arg1", "required": True}, {"name": "arg2"}]]

demisto_sdk/commands/validate/validators/BC_validators/BC111_new_required_argument_script.py

@@ -11,13 +11,13 @@
 
 ContentTypes = Script
 
-
+# This validation is similar to BC110, but specifically for scripts.
 class NewRequiredArgumentScriptValidator(BaseValidator[ContentTypes]):
     error_code = "BC111"
     description = (
-        "Validate that no new *required* argument are added to an existing script."
+        "Ensure that no new *required* arguments are added to an existing script."
     )
-    rationale = "Adding a new argument to an existing script and defining it as *required* or changing an non-required argument to be required will break backward compatibility."
+    rationale = "Adding a new required argument or changing a non-required one to required without specifying a default value breaks backward compatibility."
     error_message = "Possible backward compatibility break: You have added the following new *required* arguments: {arg_list}. Please undo the changes."
     related_field = "script.arguments"
     is_auto_fixable = False
@@ -29,24 +29,22 @@ def is_valid(self, content_items: Iterable[ContentTypes]) -> List[ValidationResu
         for content_item in content_items:
             old_content_item = content_item.old_base_content_object
 
-            for arg in content_item.args:  # type: ignore
-                current_arg_name = arg.name
-
-                if arg.required and not arg.defaultvalue:
-                    old_corresponding_arg = next(
+            arg_list = [
+                arg.name
+                for arg in content_item.args  # type: ignore
+                if arg.required
+                and not arg.defaultvalue
+                and (
+                    not next(
                         (
-                            arg
-                            for arg in old_content_item.args  # type: ignore
-                            if arg.name == current_arg_name
+                            old_arg
+                            for old_arg in old_content_item.args  # type: ignore
+                            if old_arg.name == arg.name and old_arg.required
                         ),
                         None,
                     )
-
-                    if (
-                        # If the argument is new or the argument was not required before
-                        not old_corresponding_arg
-                    ) or not old_corresponding_arg.required:
-                        arg_list.append(current_arg_name)
+                )
+            ]
             if arg_list:
                 results.append(
                     ValidationResult(