Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update required reviewer tests #37724

Merged
merged 6 commits into from
May 1, 2024
Merged

Update required reviewer tests #37724

merged 6 commits into from
May 1, 2024

Conversation

erohmensing
Copy link
Contributor

@erohmensing erohmensing commented Apr 30, 2024 •
edited
Loading

What

  • 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?

  • YES 💚
  • NO ❌

@vercel Vercel
Copy link

vercel bot commented Apr 30, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview Apr 30, 2024 10:01pm

@erohmensing erohmensing mentioned this pull request Apr 30, 2024
Merged
2 tasks
@erohmensing Graphite App
Copy link
Contributor Author

erohmensing commented Apr 30, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @erohmensing and the rest of your teammates on Graphite Graphite

@erohmensing erohmensing marked this pull request as ready for review April 30, 2024 19:17
@erohmensing erohmensing requested a review from a team as a code owner April 30, 2024 19:17
@erohmensing erohmensing mentioned this pull request Apr 30, 2024
Merged
2 tasks
bnchrch
bnchrch approved these changes May 1, 2024
View reviewed changes
Copy link
Contributor

@bnchrch bnchrch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didnt run but nothing stands out as scary.
nouns-nounish-gif

LGTM

airbyte-ci/connectors/connector_ops/tests/test_required_reviewer_checks.py
@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@erohmensing Graphite App
Copy link
Contributor Author

erohmensing commented May 1, 2024 •
edited
Loading

Merge activity

@erohmensing erohmensing merged commit af975f4 into master May 1, 2024
33 checks passed
@erohmensing erohmensing deleted the ella/04-30-update-tests branch May 1, 2024 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnchrch bnchrch bnchrch approved these changes

@alafanechere alafanechere Awaiting requested review from alafanechere

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@erohmensing @bnchrch