airbyte-ci: dynamic test step tree according to metadata #38281
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @alafanechere and the rest of your teammates on |
alafanechere
force-pushed
the
augustin/05-15-airbyte-ci_dynamic_test_step_tree_according_to_metadata
branch
2 times, most recently
from
5e8fa53 to
3fb8527
Compare
| @@ -168,7 +163,6 @@ def get_test_steps(context: ConnectorContext) -> STEP_TREE: | |||
| normalization_steps = _get_normalization_steps(context) | |||
| steps.append(normalization_steps) | |||
|
|
|||
| acceptance_test_steps = _get_acceptance_test_steps(context) | |||
| steps.append(acceptance_test_steps) | |||
| steps.append(optional_test_suites_to_run) | |||
There was a problem hiding this comment.
❗ I think this shows a case that were not supporting
- We want to remove a subset of tests from the step tree but those steps can occur in any location in the tree
A better approach is to either
1. Have a filter function you can apply to the tree that removes steps by id and outputs a new tree
def remove_steps(step_tree: STEP_TREE, remove_steps: List[str]) -> STEP_TREE:
# ...
filtered_steps = remove_steps(step_tree, steps_to_remove)
run_steps(filtered_steps)2. Make use of our skip step functionallity in run_steps
https://github.com/airbytehq/airbyte/blob/master/airbyte-ci/connectors/pipelines/pipelines/helpers/execution/run_steps.py#L81
I think #2 is the best approach
There was a problem hiding this comment.
❗ I think this shows a case that were not supporting
- We want to remove a subset of tests from the step tree but those steps can occur in any location in the tree
A better approach is to either 1. Have a filter function you can apply to the tree that removes steps by id and outputs a new tree
def remove_steps(step_tree: STEP_TREE, remove_steps: List[str]) -> STEP_TREE: # ... filtered_steps = remove_steps(step_tree, steps_to_remove) run_steps(filtered_steps)2. Make use of our skip step functionallity in run_steps https://github.com/airbytehq/airbyte/blob/master/airbyte-ci/connectors/pipelines/pipelines/helpers/execution/run_steps.py#L81
I think #2 is the best approach
@bnchrch fair point!
I changed the logic to leverage RunStepsOptions. It's definitely cleaner as:
- It allows us to not alter the concurrency settings we declare while building the STEP TREE
- Skipped tests are still rendered in the report so user can be aware that these step could run if they're enabled.
alafanechere
force-pushed
the
augustin/05-15-airbyte-ci_dynamic_test_step_tree_according_to_metadata
branch
from
3fb8527 to
bf644f4
Compare
| @@ -286,3 +295,12 @@ async def __aexit__( | |||
|
|
|||
| def create_slack_message(self) -> str: | |||
| raise NotImplementedError | |||
|
|
|||
| def _get_step_id_to_skip_according_to_metadata(self) -> List[CONNECTOR_TEST_STEP_ID]: | |||
There was a problem hiding this comment.
📚 We need a doc string here explaining the what and why.
I knew what were doing but even I struggled reading the list comprehension
| enabled_test_suites = [option["suite"] for option in self.metadata.get("connectorTestSuitesOptions", [])] | ||
| return [step_id for test_suite_name, step_id in TEST_SUITE_NAME_TO_STEP_ID.items() if test_suite_name not in enabled_test_suites] | ||
|
|
||
| def _get_updated_run_step_options(self, run_step_options: RunStepOptions) -> RunStepOptions: |
There was a problem hiding this comment.
❗ The name of the function explains the how but not the what. I'd propose we change it to
| def _get_updated_run_step_options(self, run_step_options: RunStepOptions) -> RunStepOptions: | |
| def _skip_metadata_disabled_test_suites(self, run_step_options: RunStepOptions) -> RunStepOptions: |
alafanechere
force-pushed
the
augustin/05-15-airbyte-ci_dynamic_test_step_tree_according_to_metadata
branch
from
82f85ff to
73e1c2c
Compare
alafanechere
force-pushed
the
augustin/05-15-airbyte-ci_dynamic_test_step_tree_according_to_metadata
branch
from
73e1c2c to
4832aa4
Compare
alafanechere
force-pushed
the
augustin/05-15-airbyte-ci_dynamic_test_step_tree_according_to_metadata
branch
from
4832aa4 to
c980901
Compare
alafanechere
deleted the
augustin/05-15-airbyte-ci_dynamic_test_step_tree_according_to_metadata
branch
What
Closes https://github.com/airbytehq/airbyte-internal-issues/issues/7552
Make
airbyte-ciconsume theconnectorTestSuitesOptionsfrom connector metadata to conditionally run test suites declared in there.How
get_test_suites_to_run_according_to_metadatafunction taking the metadata and the optional steps to runUser Impact
After merging this we should ask all connector developers to update their branch so that they get the freshest connector metadata introduced in #38188 . Critical tests would be skipped otherwise...