Refactoring of tests for UpdateApprovalsService and rename it to SyncScanFindingRulesService

Everyone can contribute. Help move this issue forward while earning points, leveling up and collecting rewards.

• Close this issue

Why are we doing this work

Tests for UpdateApprovalsService have grown and the current structure makes it difficult to extend and ensure we cover the whole functionality with tests.

To improve the quality and confidence when making further changes to the approval policies, we'd benefit from refactoring of these tests.

The main challenge is that we're mixing various things in the top-level contexts such as:

• when target pipeline is nil
• when there are preexisting findings that exceed the allowed limit, within that we have a sub-context again for when target pipeline is nil
• when there are no required approvals
• with multiple pipeline
• with merged results pipeline
• when the approval rule has vulnerability attributes

Relevant links

Implementation plan

The structure should cover:

• how the source and target pipelines are looked up
• how each of the policy options are applied
• how the rules are updated as a result of violation / no-violation

Therefore, I propose the following the structure:

• with new findings
  • various describe blocks, in each we'd check for approval rule updates, violations, bot comment, logging
    • based on the approval rules to capture the fact that we'd using filter_newly_detected_rules
    • based on the source pipeline (head or related, target pipeline not important in this case) to capture ways how we look for the source pipeline
    • based on the policy attributes, such as vulnerability_states, vulnerabilities_allowed, scanners, etc. to capture ways how a policy can be fine-tuned
  • each of the cases above would use shared examples for everything that happens with an approval rule when violated (logs, violations, scan removed, fail_open), and when not (violations removed, rule updated)
• with new and preexisting findings
  • same structure as for with new findings
  • additional cases based on the various options of target pipeline to capture how we look for it and how the total of new + existing can exceed vulnerabilities_allowed
• with only preexisting findings
  • no-op since this is handled by SyncPreexistingStatesApprovalRulesService

We should also rename the service to SyncScanFindingRulesService to better reflect what it does.

Verification steps

1. Verify that tests cover all existing cases and ideally any further cases that could be missing now
2. Verify the test suite duration doesn't increase drastically (ensure we use optimizations using let_it_be where appropriate)