Fixing test playbook validations (#386)

* Fixing test playbook validations

Co-authored-by: halpert <[email protected]>

demisto_sdk/commands/common/hook_validations/base_validator.py

@@ -8,8 +8,8 @@
  StructureValidator
 from demisto_sdk.commands.common.tools import (_get_file_id,
  get_latest_release_notes_text,
- get_not_registered_tests,
  get_release_notes_file_path,
+ is_test_config_match,
  print_error, run_command)
 
 
@@ -130,18 +130,7 @@ def _load_conf_file(self):
  with open(self.CONF_PATH) as data_file:
  return json.load(data_file)
 
- def are_tests_configured(self) -> bool:
- """
- Checks if a file (playbook or integration) has a TestPlaybook and if the TestPlaybook is configured in conf.json
- And prints an error message accordingly
- """
- file_type = self.structure_validator.scheme_name
- tests = self.current_file.get('tests', [])
- if not self.yml_has_test_key(tests, file_type):
- return False
- return self.tests_registered_in_conf_json_file(tests)
-
- def tests_registered_in_conf_json_file(self, test_playbooks: list) -> bool:
+ def are_tests_registered_in_conf_json_file(self, test_playbooks: list) -> bool:
  """
  Checking if test playbooks are configured in 'conf.json' unless 'No tests' is in test playbooks.
  If 'No tests' is not in test playbooks and there is a test playbook that is not configured: Will print's
@@ -155,26 +144,43 @@ def tests_registered_in_conf_json_file(self, test_playbooks: list) -> bool:
  no_tests_explicitly = any(test for test in test_playbooks if 'no test' in test.lower())
  if no_tests_explicitly:
  return True
+
  conf_json_tests = self._load_conf_file()['tests']
 
  content_item_id = _get_file_id(self.structure_validator.scheme_name, self.current_file)
  file_type = self.structure_validator.scheme_name
- not_registered_tests = get_not_registered_tests(conf_json_tests, content_item_id, file_type, test_playbooks)
- if not_registered_tests:
- file_type = self.structure_validator.scheme_name
- if file_type == 'integration':
- missing_test_configurations = json.dumps([
- {'integrations': content_item_id, 'playbookID': test} for test in not_registered_tests
- ], indent=4).strip('[]')
- else:
- missing_test_configurations = json.dumps([
- {'playbookID': test} for test in not_registered_tests
- ], indent=4).strip('[]')
- error_message = \
- f'The following TestPlaybooks are not registered in {self.CONF_PATH} file.\n' \
- f'Please add\n{missing_test_configurations}\nto {self.CONF_PATH} path under \'tests\' key.'
- print_error(error_message)
- return False
+ # Test playbook case
+
+ if 'TestPlaybooks' in self.file_path and file_type == 'playbook':
+ is_configured_test = any(test_config for test_config in conf_json_tests if
+ is_test_config_match(test_config, test_playbook_id=content_item_id))
+ if not is_configured_test:
+ missing_test_playbook_configurations = json.dumps({'playbookID': content_item_id}, indent=4)
+ missing_integration_configurations = json.dumps(
+ {'integrations': '<integration ID>', 'playbookID': content_item_id},
+ indent=4)
+ error_message = \
+ f'The TestPlaybook {content_item_id} is not registered in {self.CONF_PATH} file.\n' \
+ f'Please add\n{missing_test_playbook_configurations}\n' \
+ f'or if this test playbook is for an integration\n{missing_integration_configurations}\n' \
+ f'to {self.CONF_PATH} path under \'tests\' key.'
+ print_error(error_message)
+ return False
+
+ # Integration case
+ elif file_type == 'integration':
+ is_configured_test = any(
+ test_config for test_config in conf_json_tests if is_test_config_match(test_config,
+ integration_id=content_item_id))
+ if not is_configured_test:
+ missing_test_playbook_configurations = json.dumps(
+ {'integrations': content_item_id, 'playbookID': '<TestPlaybook ID>'},
+ indent=4)
+ error_message = \
+ f'The following TestPlaybooks are not registered in {self.CONF_PATH} file.\n' \
+ f'Please add\n{missing_test_playbook_configurations}\nto {self.CONF_PATH} path under \'tests\' key.'
+ print_error(error_message)
+ return False
  return True
 
  def yml_has_test_key(self, test_playbooks: list, file_type: str) -> bool:
@@ -183,7 +189,7 @@ def yml_has_test_key(self, test_playbooks: list, file_type: str) -> bool:
  If not: prints an error message according to the file type and return the check result
  Args:
  test_playbooks: The yml file's list of test playbooks
- file_type: The file type, could be a script, an integration or a playbook.
+ file_type: The file type, could be an integration or a playbook.
 
  Returns:
  True if tests are configured (not None and not an empty list) otherwise return False.

demisto_sdk/commands/common/hook_validations/integration.py

@@ -84,6 +84,17 @@ def is_valid_file(self, validate_rn: bool = True) -> bool:
  ]
  return all(answers)
 
+ def are_tests_configured(self) -> bool:
+ """
+ Checks if the integration has a TestPlaybook and if the TestPlaybook is configured in conf.json
+ And prints an error message accordingly
+ """
+ file_type = self.structure_validator.scheme_name
+ tests = self.current_file.get('tests', [])
+ if not self.are_tests_registered_in_conf_json_file(tests):
+ return self.yml_has_test_key(tests, file_type)
+ return True
+
  def is_valid_beta_integration(self, validate_rn: bool = True) -> bool:
  """Check whether the beta Integration is valid or not, update the _is_valid field to determine that
  Args:

demisto_sdk/commands/common/hook_validations/playbook.py

@@ -43,6 +43,18 @@ def is_valid_playbook(self, is_new_playbook: bool = True, validate_rn: bool = Tr
 
  return answers
 
+ def are_tests_configured(self) -> bool:
+ """
+ Checks if the playbook has a TestPlaybook and if the TestPlaybook is configured in conf.json
+ And prints an error message accordingly
+ """
+ if 'TestPlaybooks' in self.file_path:
+ return True
+
+ file_type = self.structure_validator.scheme_name
+ tests = self.current_file.get('tests', [])
+ return self.yml_has_test_key(tests, file_type)
+
  def is_id_equals_name(self): # type: () -> bool
  """Check whether the playbook ID is equal to its name.
 

demisto_sdk/commands/common/hook_validations/script.py

@@ -62,7 +62,6 @@ def is_valid_file(self, validate_rn=True):
  self.is_id_equals_name(),
  self.is_docker_image_valid(),
  self.is_valid_pwsh(),
- self.are_tests_configured()
  ])
  # check only on added files
  if not self.old_file:

demisto_sdk/commands/common/tests/base_validator_test.py

@@ -3,11 +3,11 @@
  BaseValidator
 from demisto_sdk.commands.common.hook_validations.structure import \
  StructureValidator
-from demisto_sdk.commands.common.tools import (find_test_match,
- get_not_registered_tests)
+from demisto_sdk.commands.common.tools import (get_not_registered_tests,
+ is_test_config_match)
 from demisto_sdk.tests.constants_test import (
- INVALID_INTEGRATION_WITH_NO_TEST_PLAYBOOK, SCRIPT_WITH_PLAYBOOK,
- VALID_INTEGRATION_TEST_PATH, VALID_TEST_PLAYBOOK_PATH)
+ INVALID_INTEGRATION_WITH_NO_TEST_PLAYBOOK, VALID_INTEGRATION_TEST_PATH,
+ VALID_TEST_PLAYBOOK_PATH)
 
 HAS_TESTS_KEY_UNPUTS = [
  (VALID_INTEGRATION_TEST_PATH, 'integration', True),
@@ -63,27 +63,13 @@ def test_yml_has_test_key(file_path, schema, expected):
  'integration',
  False
  ),
- (
- {'integrations': ['integration1', 'integration2'], 'playbookID': 'playbook1'},
- 'integration1',
- 'playbook2',
- 'integration',
- False
- ),
  (
  {'playbookID': 'playbook1'},
- 'playbook',
+ '',
  'playbook1',
  'playbook',
  True
  ),
- (
- {'playbookID': 'playbook1'},
- 'some-script',
- 'playbook1',
- 'script',
- True
- ),
 
 ]
 
@@ -101,7 +87,7 @@ def test_find_test_match(test_config, integration_id, test_playbook_id, expected
  Then
  - Ensure the method 'find_test_match' return answer accordingly
  """
- assert find_test_match(test_config, test_playbook_id, integration_id, file_type) == expected
+ assert is_test_config_match(test_config, test_playbook_id, integration_id) == expected
 
 
 NOT_REGISTERED_TESTS_INPUT = [
@@ -123,7 +109,7 @@ def test_find_test_match(test_config, integration_id, test_playbook_id, expected
  VALID_INTEGRATION_TEST_PATH,
  'integration',
  [{'integrations': 'PagerDuty v2', 'playbookID': 'Playbook'}],
- 'PagerDuty v2',
+ 'PagerDuty v3',
  ['PagerDuty Test']
  ),
  (
@@ -140,21 +126,6 @@ def test_find_test_match(test_config, integration_id, test_playbook_id, expected
  'Account Enrichment',
  ['PagerDuty Test']
  ),
- (
- SCRIPT_WITH_PLAYBOOK,
- 'script',
- [{'integrations': 'TestCreateDuplicates', 'playbookID': 'PagerDuty Test'}],
- 'TestCreateDuplicates',
- []
- ),
- (
- SCRIPT_WITH_PLAYBOOK,
- 'script',
- [{'integrations': 'TestCreateDuplicates', 'playbookID': 'other test'}],
- 'TestCreateDuplicates',
- ['PagerDuty Test']
- )
-
 ]
 
 

demisto_sdk/commands/common/tools.py

@@ -807,33 +807,41 @@ def get_depth(data: Any) -> int:
  return 0
 
 
-def find_test_match(test_config: dict, test_playbook_id: str, content_item_id: str, file_type: str) -> bool:
+def is_test_config_match(test_config: dict, test_playbook_id: str = '', integration_id: str = '') -> bool:
  """
  Given a test configuration from conf.json file, this method checks if the configuration is configured for the
- test playbook with content item.
+ test playbook or for integration_id.
  Since in conf.json there could be test configurations with 'integrations' as strings or list of strings
  the type of test_configurations['integrations'] is checked in first and the match according to the type.
  If file type is not an integration- will return True if the test_playbook id matches playbookID.
  Args:
- file_type: The file type. can be 'integration', 'script', 'playbook'.
  test_config: A test configuration from conf.json file under 'tests' key.
+ file_type: The file type. can be 'integration', 'playbook'.
  test_playbook_id: A test playbook ID.
- content_item_id: A content item ID, could be a script, an integration or a playbook.
-
+ integration_id: An integration ID.
+ If both test_playbook_id and integration_id are given will look for a match of both, else will look for match
+ of either test playbook id or integration id
  Returns:
  True if the test configuration contains the test playbook and the content item or False if not
  """
- if test_playbook_id != test_config.get('playbookID'):
- return False
- if file_type != 'integration':
- return True
-
+ test_playbook_match = test_playbook_id == test_config.get('playbookID')
  test_integrations = test_config.get('integrations')
  if isinstance(test_integrations, list):
- return any(
- test_integration for test_integration in test_integrations if test_integration == content_item_id)
+ integration_match = any(
+ test_integration for test_integration in test_integrations if test_integration == integration_id)
  else:
- return test_integrations == content_item_id
+ integration_match = test_integrations == integration_id
+ # If both playbook id and integration id are given
+ if integration_id and test_playbook_id:
+ return test_playbook_match and integration_match
+
+ # If only integration id is given
+ if integration_id:
+ return integration_match
+
+ # If only test playbook is given
+ if test_playbook_id:
+ return test_playbook_match
 
 
 def get_not_registered_tests(conf_json_tests: list, content_item_id: str, file_type: str, test_playbooks: list) -> list:
@@ -842,20 +850,24 @@ def get_not_registered_tests(conf_json_tests: list, content_item_id: str, file_t
  Args:
  conf_json_tests: the 'tests' value of 'conf.json file
  content_item_id: A content item ID, could be a script, an integration or a playbook.
- file_type: The file type, could be a script, an integration or a playbook.
+ file_type: The file type, could be an integration or a playbook.
  test_playbooks: The yml file's list of test playbooks
 
  Returns:
  A list of TestPlaybooks not configured
  """
  not_registered_tests = []
  for test in test_playbooks:
- test_registered_in_conf_json = any(
- test_config for test_config in conf_json_tests if find_test_match(test_config,
- test,
- content_item_id,
- file_type)
- )
+ if file_type == 'playbook':
+ test_registered_in_conf_json = any(
+ test_config for test_config in conf_json_tests if is_test_config_match(test_config,
+ test_playbook_id=test)
+ )
+ else:
+ test_registered_in_conf_json = any(
+ test_config for test_config in conf_json_tests if is_test_config_match(test_config,
+ integration_id=content_item_id)
+ )
  if not test_registered_in_conf_json:
  not_registered_tests.append(test)
  return not_registered_tests

demisto_sdk/commands/format/update_script.py

@@ -25,7 +25,6 @@ def run_format(self) -> int:
  try:
  super().update_yml()
  self.update_tests()
- self.update_conf_json('script')
  self.save_yml_to_destination_file()
  return SUCCESS_RETURN_CODE
  except Exception:

demisto_sdk/commands/validate/tests/validators_test.py

@@ -42,15 +42,16 @@
  INVALID_PLAYBOOK_PATH, INVALID_PLAYBOOK_PATH_FROM_ROOT,
  INVALID_REPUTATION_PATH, INVALID_SCRIPT_PATH, INVALID_WIDGET_PATH,
  LAYOUT_TARGET, PLAYBOOK_TARGET, REPUTATION_TARGET,
- SCRIPT_RELEASE_NOTES_TARGET, SCRIPT_TARGET, VALID_BETA_INTEGRATION,
- VALID_BETA_PLAYBOOK_PATH, VALID_DASHBOARD_PATH, VALID_INCIDENT_FIELD_PATH,
- VALID_INCIDENT_TYPE_PATH, VALID_INTEGRATION_ID_PATH,
- VALID_INTEGRATION_TEST_PATH, VALID_LAYOUT_PATH, VALID_MD,
- VALID_MULTI_LINE_CHANGELOG_PATH, VALID_MULTI_LINE_LIST_CHANGELOG_PATH,
- VALID_NO_HIDDEN_PARAMS, VALID_ONE_LINE_CHANGELOG_PATH,
- VALID_ONE_LINE_LIST_CHANGELOG_PATH, VALID_PACK, VALID_PLAYBOOK_CONDITION,
- VALID_REPUTATION_PATH, VALID_SCRIPT_PATH, VALID_TEST_PLAYBOOK_PATH,
- VALID_WIDGET_PATH, WIDGET_TARGET)
+ SCRIPT_RELEASE_NOTES_TARGET, SCRIPT_TARGET, TEST_PLAYBOOK,
+ VALID_BETA_INTEGRATION, VALID_BETA_PLAYBOOK_PATH, VALID_DASHBOARD_PATH,
+ VALID_INCIDENT_FIELD_PATH, VALID_INCIDENT_TYPE_PATH,
+ VALID_INTEGRATION_ID_PATH, VALID_INTEGRATION_TEST_PATH, VALID_LAYOUT_PATH,
+ VALID_MD, VALID_MULTI_LINE_CHANGELOG_PATH,
+ VALID_MULTI_LINE_LIST_CHANGELOG_PATH, VALID_NO_HIDDEN_PARAMS,
+ VALID_ONE_LINE_CHANGELOG_PATH, VALID_ONE_LINE_LIST_CHANGELOG_PATH,
+ VALID_PACK, VALID_PLAYBOOK_CONDITION, VALID_REPUTATION_PATH,
+ VALID_SCRIPT_PATH, VALID_TEST_PLAYBOOK_PATH, VALID_WIDGET_PATH,
+ WIDGET_TARGET)
 from mock import patch
 
 
@@ -355,7 +356,8 @@ def test_is_valid_rn(self, mocker, file_path, file_type):
  mocker.patch.object(DashboardValidator, 'is_id_equals_name', return_value=True)
  mocker.patch.object(ReputationValidator, 'is_id_equals_details', return_value=True)
  mocker.patch.object(IntegrationValidator, 'is_valid_beta', return_value=True)
- mocker.patch.object(BaseValidator, 'are_tests_configured', return_value=True)
+ mocker.patch.object(IntegrationValidator, 'are_tests_configured', return_value=True)
+ mocker.patch.object(PlaybookValidator, 'are_tests_configured', return_value=True)
  file_validator = FilesValidator(validate_conf_json=False)
  file_validator.validate_added_files(file_path, file_type)
  assert file_validator._is_valid
@@ -428,13 +430,14 @@ def test_pack_validation(self):
  assert file_validator._is_valid is False
 
  ARE_TEST_CONFIGURED_TEST_INPUT = [
- (VALID_INTEGRATION_TEST_PATH, True),
- (INVALID_INTEGRATION_NO_TESTS, False),
- (INVALID_INTEGRATION_NON_CONFIGURED_TESTS, False)
+ (VALID_INTEGRATION_TEST_PATH, 'integration', True),
+ (INVALID_INTEGRATION_NO_TESTS, 'integration', False),
+ (INVALID_INTEGRATION_NON_CONFIGURED_TESTS, 'integration', True),
+ (TEST_PLAYBOOK, 'playbook', False)
  ]
 
- @pytest.mark.parametrize('file_path, expected', ARE_TEST_CONFIGURED_TEST_INPUT)
- def test_are_tests_configured(self, file_path, expected):
+ @pytest.mark.parametrize('file_path, file_type, expected', ARE_TEST_CONFIGURED_TEST_INPUT)
+ def test_are_tests_configured(self, file_path, file_type, expected):
  # type: (str, bool) -> None
  """
  Given
@@ -446,6 +449,6 @@ def test_are_tests_configured(self, file_path, expected):
  Then
  - validator return the correct answer accordingly
  """
- structure_validator = StructureValidator(file_path, predefined_scheme='integration')
- validator = BaseValidator(structure_validator)
+ structure_validator = StructureValidator(file_path, predefined_scheme=file_type)
+ validator = IntegrationValidator(structure_validator)
  assert validator.are_tests_configured() == expected

demisto_sdk/tests/constants_test.py

@@ -9,6 +9,7 @@
 INVALID_INTEGRATION_NO_TESTS = f'{GIT_ROOT}/demisto_sdk/tests/test_files/non-valid-integration-no-test-playbooks.yml'
 INVALID_INTEGRATION_NON_CONFIGURED_TESTS = f'{GIT_ROOT}/demisto_sdk/tests/test_files/' \
  f'non-valid-integration-test-not-configured.yml'
+TEST_PLAYBOOK = f'{GIT_ROOT}/demisto_sdk/tests/test_files/playbook-TestPlaybooks.yml'
 VALID_INTEGRATION_TEST_PATH = f"{GIT_ROOT}/demisto_sdk/tests/test_files/integration-test.yml"
 INVALID_INTEGRATION_WITH_NO_TEST_PLAYBOOK = 'demisto_sdk/tests/test_files/integration-test-with-no-test-playbook.yml'
 VALID_INTEGRATION_ID_PATH = f"{GIT_ROOT}/demisto_sdk/tests/test_files/integration-valid-id-test.yml"