Skip to content

Commit

Permalink
Update required reviewer tests (#37724)
Browse files Browse the repository at this point in the history
## What
<!--
* Describe what the change is solving. Link all GitHub issues related to this change.
-->
* Updates the tests to be explicit about exact matching the teams that come out of the requiements file instead of just checking whether any of the expected teams were in it. Makes it easier to know which groups will be tagged for a given test. 
* change usage of `mock_diffed_branches` because it looks unused in the methods, and i tried removing it, but it does need to run before every test 😅
* Update test expectations accordingly to list which groups are tagged in requirements

## Can this PR be safely reverted and rolled back?
<!--
* If unsure, leave it blank.
-->
- [x] YES 💚
- [ ] NO ❌
  • Loading branch information
erohmensing committed May 1, 2024
1 parent 914f044 commit af975f4
Showing 1 changed file with 23 additions and 22 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
from connector_ops import required_reviewer_checks


@pytest.fixture
# This fixture ensure that the remote CI works the same way local CI does
@pytest.fixture(autouse=True)
def mock_diffed_branched(mocker):
airbyte_repo = git.Repo(search_parent_directories=True)
mocker.patch.object(required_reviewer_checks.utils, "DIFFED_BRANCH", airbyte_repo.active_branch)
Expand Down Expand Up @@ -57,6 +58,7 @@ def not_strategic_test_strictness_level_change_expected_team(tmp_path, pokeapi_a

@pytest.fixture
def not_strategic_bypass_reason_file_change_expected_team(tmp_path, pokeapi_acceptance_test_config_path):
# The bypass reason logic explicitly only checks for bypass reasons on strategic connectors
expected_teams = []
backup_path = tmp_path / "non_strategic_acceptance_test_config.backup"
shutil.copyfile(pokeapi_acceptance_test_config_path, backup_path)
Expand Down Expand Up @@ -90,7 +92,9 @@ def strategic_connector_file_change_expected_team(tmp_path, strategic_connector_

@pytest.fixture
def strategic_connector_backward_compatibility_file_change_expected_team(tmp_path, strategic_connector_file):
expected_teams = list(required_reviewer_checks.BACKWARD_COMPATIBILITY_REVIEWERS)
expected_teams = list(
required_reviewer_checks.STRATEGIC_PYTHON_CONNECTOR_REVIEWERS.union(required_reviewer_checks.BACKWARD_COMPATIBILITY_REVIEWERS)
)
backup_path = tmp_path / "strategic_acceptance_test_config.backup"
shutil.copyfile(strategic_connector_file, backup_path)
with open(strategic_connector_file, "a") as strategic_acceptance_test_config_file:
Expand All @@ -101,7 +105,9 @@ def strategic_connector_backward_compatibility_file_change_expected_team(tmp_pat

@pytest.fixture
def strategic_connector_bypass_reason_file_change_expected_team(tmp_path, strategic_connector_file):
expected_teams = list(required_reviewer_checks.BYPASS_REASON_REVIEWERS)
expected_teams = list(
required_reviewer_checks.STRATEGIC_PYTHON_CONNECTOR_REVIEWERS.union(required_reviewer_checks.BYPASS_REASON_REVIEWERS)
)
backup_path = tmp_path / "strategic_acceptance_test_config.backup"
shutil.copyfile(strategic_connector_file, backup_path)
with open(strategic_connector_file, "a") as strategic_acceptance_test_config_file:
Expand All @@ -112,7 +118,9 @@ def strategic_connector_bypass_reason_file_change_expected_team(tmp_path, strate

@pytest.fixture
def strategic_connector_test_strictness_level_file_change_expected_team(tmp_path, strategic_connector_file):
expected_teams = list(required_reviewer_checks.TEST_STRICTNESS_LEVEL_REVIEWERS)
expected_teams = list(
required_reviewer_checks.STRATEGIC_PYTHON_CONNECTOR_REVIEWERS.union(required_reviewer_checks.TEST_STRICTNESS_LEVEL_REVIEWERS)
)
backup_path = tmp_path / "strategic_acceptance_test_config.backup"
shutil.copyfile(strategic_connector_file, backup_path)
with open(strategic_connector_file, "a") as strategic_acceptance_test_config_file:
Expand Down Expand Up @@ -145,7 +153,8 @@ def verify_requirements_file_was_generated(captured: str):
def verify_review_requirements_file_contains_expected_teams(requirements_file_path: str, expected_teams: List):
with open(requirements_file_path, "r") as requirements_file:
requirements = yaml.safe_load(requirements_file)
assert any([r["teams"] == expected_teams for r in requirements])
all_required_teams = set().union(*(r["teams"] for r in requirements))
assert all_required_teams == set(expected_teams)


def check_review_requirements_file(capsys, expected_teams: List):
Expand All @@ -159,49 +168,41 @@ def check_review_requirements_file(capsys, expected_teams: List):
verify_review_requirements_file_contains_expected_teams(requirements_file_path, expected_teams)


def test_find_mandatory_reviewers_backward_compatibility(
mock_diffed_branched, capsys, not_strategic_backward_compatibility_change_expected_team
):
def test_find_mandatory_reviewers_backward_compatibility(capsys, not_strategic_backward_compatibility_change_expected_team):
check_review_requirements_file(capsys, not_strategic_backward_compatibility_change_expected_team)


def test_find_mandatory_reviewers_test_strictness_level(
mock_diffed_branched, capsys, not_strategic_test_strictness_level_change_expected_team
):
def test_find_mandatory_reviewers_test_strictness_level(capsys, not_strategic_test_strictness_level_change_expected_team):
check_review_requirements_file(capsys, not_strategic_test_strictness_level_change_expected_team)


def test_find_mandatory_reviewers_not_strategic_bypass_reason(
mock_diffed_branched, capsys, not_strategic_bypass_reason_file_change_expected_team
):
def test_find_mandatory_reviewers_not_strategic_bypass_reason(capsys, not_strategic_bypass_reason_file_change_expected_team):
check_review_requirements_file(capsys, not_strategic_bypass_reason_file_change_expected_team)


def test_find_mandatory_reviewers_ga(mock_diffed_branched, capsys, strategic_connector_file_change_expected_team):
def test_find_mandatory_reviewers_ga(capsys, strategic_connector_file_change_expected_team):
check_review_requirements_file(capsys, strategic_connector_file_change_expected_team)


def test_find_mandatory_reviewers_strategic_backward_compatibility(
mock_diffed_branched, capsys, strategic_connector_backward_compatibility_file_change_expected_team
capsys, strategic_connector_backward_compatibility_file_change_expected_team
):
check_review_requirements_file(capsys, strategic_connector_backward_compatibility_file_change_expected_team)


def test_find_mandatory_reviewers_strategic_bypass_reason(
mock_diffed_branched, capsys, strategic_connector_bypass_reason_file_change_expected_team
):
def test_find_mandatory_reviewers_strategic_bypass_reason(capsys, strategic_connector_bypass_reason_file_change_expected_team):
check_review_requirements_file(capsys, strategic_connector_bypass_reason_file_change_expected_team)


def test_find_mandatory_reviewers_strategic_test_strictness_level(
mock_diffed_branched, capsys, strategic_connector_test_strictness_level_file_change_expected_team
capsys, strategic_connector_test_strictness_level_file_change_expected_team
):
check_review_requirements_file(capsys, strategic_connector_test_strictness_level_file_change_expected_team)


def test_find_mandatory_reviewers_breaking_change_release(mock_diffed_branched, capsys, test_breaking_change_release_expected_team):
def test_find_mandatory_reviewers_breaking_change_release(capsys, test_breaking_change_release_expected_team):
check_review_requirements_file(capsys, test_breaking_change_release_expected_team)


def test_find_mandatory_reviewers_no_tracked_changed(mock_diffed_branched, capsys, not_strategic_not_tracked_change_expected_team):
def test_find_mandatory_reviewers_no_tracked_changed(capsys, not_strategic_not_tracked_change_expected_team):
check_review_requirements_file(capsys, not_strategic_not_tracked_change_expected_team)

0 comments on commit af975f4

Please sign in to comment.