Regression tests: run with `airbyte-ci` #37440

clnoll · 2024-04-19T18:34:35Z

Adds a new command and pipeline for running regression tests in airbyte-ci.

This requires us to make a couple of changes to the live tests to bypass prompts. For this reason, test artifacts are cleaned up immediately when the test run is complete. If artifacts are needed the user should run this with pytest (as documented here). (When run in Github Actions, artifacts will be uploaded to GCS.)

TODO:

Use a service account for gcloud auth.

@alafanechere responding to a couple of your questions/comments:

Can we make the RegressionTests step inherit from PytestStep?

PytestStep mostly concerns itself with running tests that exist in a connector container. We'd have to refactor/overwrite a lot of the existing functionality to make it reusable here.

I don't think there's a need for a specific regression test command in airbyte ci. I believe we should use airbyte-ci connectors test. If we want to pass custom parameters to regression test this can be done via parameters passing: `--regression-test.--=

To me it feels premature to have regression tests be part of the testsuite so IMO we should hold off until we've decided on a cadence to run them on and can verify that we're getting meaningful, non-noisy results. I'm not personally totally sure we'll ever want to run them as part of "normal" tests, so would rather give them their own command than pollute test with conditional logic.

vercel · 2024-04-19T18:34:42Z

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 25, 2024 9:41pm

alafanechere · 2024-04-23T07:18:28Z

To me it feels premature to have regression tests be part of the testsuite so IMO we should hold off until we've decided on a cadence to run them on and can verify that we're getting meaningful, non-noisy results. I'm not personally totally sure we'll ever want to run them as part of "normal" tests, so would rather give them their own command than pollute test with conditional logic.

@clnoll this makes sense to me and I think this is where I miss details to give a review: how do you plan to orchestrate the CI run?

I'd like to avoid the proliferation of commands on airbyte-ci and would prefer:

Changing our existing "normal test suites CI command" from airbyte-ci connectors --modified test to airbyte-connectors --modified test --skip-step=regression
Declaring a specific regression test workflow which runs airbyte-connectors --modified test --only-step=regression

This will make it easier to eventually integrate the regression test as part of the automated test suite and avoid introducing new command and code.

clnoll · 2024-04-23T13:46:06Z

@alafanechere my hesitation with --skip-step=regression is that for anyone who is running airbyte-ci connectors test locally, they'll have to actively remember to skip regression tests, or else we could end up running them many many times. Since we haven't implemented rate limiting per connection ID yet and don't have a perfect story for what it means for a regression test to fail, I don't think we should allow this to happen.

If we're agreed on that, I'm happy either keeping them in airbyte-ci connectors test but defaulting to skipping them (which could get a little bit ugly in the code but would address my concern) or give them a separate command as they have in this PR. I personally still have a slight preference for a separate command because I don't see them ever being part of the normal CI test cadence, but can understand the aversion to giving airbyte-ci too many commands and don't feel too strongly, as long as we have sensible defaults.

alafanechere · 2024-04-23T15:29:01Z

If we're agreed on that, I'm happy either keeping them in airbyte-ci connectors test but defaulting to skipping them (which could get a little bit ugly in the code but would address my concern) or give them a separate command as they have in this PR. I personally still have a slight preference for a separate command because I don't see them ever being part of the normal CI test cadence, but can understand the aversion to giving airbyte-ci too many commands and don't feel too strongly, as long as we have sensible defaults.

I think a reasonable tradeoff could be to introduce a --run-regression-tests flag to the test command. It would default to False to address the "do no run it locally" concern. I'd love to keep the regression test run in the main test pipeline so that enabling it later by default will be very easy and we do not risk to introduce diverging code paths to refactor later.

We could also make the default --skip-step value be ["regression"]. I think I prefer this 😄

clnoll · 2024-04-23T18:32:49Z

Just closing the loop on the conversation - we've decided to keep regression tests in the main test pipeline.

To avoid regression tests getting run unintentionally, we'll always add them to the --skip-step list, so they'll only run when explicitly specified via --only-step.

clnoll · 2024-04-23T19:50:02Z

airbyte-ci/connectors/live-tests/src/live_tests/commons/utils.py

@@ -96,10 +96,9 @@ async def get_connector_container(dagger_client: dagger.Client, image_name_with_
    # If a container_id.txt file is available, we'll use it to load the connector container
    # We use a txt file as container ids can be too long to be passed as env vars
    # It's used for dagger-in-dagger use case with airbyte-ci, when the connector container is built via an upstream dagger operation
-    connector_container_id_path = Path("/tmp/container_id.txt")
-    if connector_container_id_path.exists():
-        # If the CONNECTOR_CONTAINER_ID env var is set, we'll use it to load the connector container


We're not getting it from the env var so I deleted this comment.

clnoll · 2024-04-23T20:05:44Z

airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/connectors/test/steps/common.py

+        self.should_read_with_state = self._get_item_or_default(options, "should-read-with-state", True)
+
+    @staticmethod
+    def _get_item_or_default(options: Dict[str, Any], key: str, default: Any):


@alafanechere is this something we're already doing elsewhere?

alafanechere

I'm mainly requesting changes for the minor suggestions. LGTM logic wise.
Can you please bump both project versions and changelogs 🙏

alafanechere · 2024-04-24T14:45:17Z

airbyte-ci/connectors/live-tests/src/live_tests/commons/segment_tracking.py

+    if user_id:
+        analytics.identify(user_id)
+    else:
+        user_id = "airbyte-ci"


I would find it cleaner to explicitely pass airbyte-ci as the user id.
The user_id could also be fetched from an environment variable as it'd make it possible to track different CI use of the tool.

alafanechere · 2024-04-24T14:53:14Z

airbyte-ci/connectors/live-tests/src/live_tests/regression_tests/conftest.py

-    start_timestamp = int(time.time())
+    start_timestamp = int(config.getoption("--start-timestamp") or time.time())


I'm not sure why we want to override the start_timestamp? Is it to get predictable session path?
I would suggest creating a function which would produce the test_artifacts_directory and uses the github run id if we're in CI.
Something like:

def get_artifacts_directory(start_timestamp): suffix = os.environ["GITHUB_RUN_ID"] if "GITHUB_RUN_ID" in os.environ else str(start_timestamp) return MAIN_OUTPUT_DIRECTORY / f"session_{suffix}"

Is it to get predictable session path?

Yes that's right.
Using "GITHUB_RUN_ID" will work when this is run in GhA, but when we run it locally with airbyte-ci we don't have that. I'm not seeing anything else in the environment that makes sense to use.

I agree it feels a little hacky to pass in start time, but we do need something that's known by both the external dagger process and the conftest.

To address this I made a change to pass --run-id to pytest instead of --start-timestamp. Then I let the dagger process look for GITHUB_RUN_ID and fall back to a start timestamp if it's not present. Does this look better to you?

airbyte-ci/connectors/live-tests/src/live_tests/regression_tests/conftest.py

alafanechere · 2024-04-24T15:01:43Z

airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/connectors/build_image/steps/common.py

I'm not sure why this file changed

airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/connectors/commands.py

alafanechere · 2024-04-24T15:10:50Z

airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/connectors/test/steps/common.py

+        self.should_read_with_state = self._get_item_or_default(options, "should-read-with-state", True)
+
+    @staticmethod
+    def _get_item_or_default(options: Dict[str, Any], key: str, default: Any):


I don't think so but it can go to the helpers / utils. I'd be more inclined to use dpath instead of writing this kind of function.

Updated to use dpath and made it a staticmethod on RunStepOptions.

alafanechere · 2024-04-24T15:11:38Z

airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/connectors/test/steps/common.py

+        """
+        super().__init__(context)
+        self.connector_image = context.docker_image.split(":")[0]
+        print(self.context.run_step_options.step_params)


Can we avoid printing here in case there's a secret?

Yep, deleted.

alafanechere · 2024-04-24T15:14:06Z

airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/connectors/test/steps/common.py

+        await container.directory(regression_tests_artifacts_dir).export(regression_tests_artifacts_dir)
+        path_to_report = f"{regression_tests_artifacts_dir}/session_{int(start_timestamp)}/report.html"


Can we just export the report in CI to minimize sensitive data exposure?

Sure, done.

alafanechere · 2024-04-24T15:15:22Z

airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/connectors/test/steps/common.py

+                .with_mounted_file("/root/.ssh/id_rsa", self.dagger_client.host().file(str(Path("~/.ssh/id_rsa").expanduser())))  # TODO
+                .with_mounted_file(
+                    "/root/.ssh/known_hosts", self.dagger_client.host().file(str(Path("~/.ssh/known_hosts").expanduser()))  # TODO
+                )


not 💯 sure this will work in GHA runners

It doesn't 😄 . But I'll make the needed update in the gha PR instead of here.

alafanechere · 2024-04-24T15:16:57Z

airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/connectors/test/steps/common.py

+        container = (
+            (
+                container.with_exec(["apt-get", "update"])
+                .with_exec(["apt-get", "install", "-y", "git", "openssh-client", "curl", "docker.io"])


It's a bit annoying that we have to install git and openssh-client just because we have a private dependency to install...

I agree. I'm trying to make this go away in the gha follow up PR.

But I did not mean I have a better suggestion. 😄

alafanechere

Thanks for your replies and changes 🙏 Please bump the versions and add changelog entries before merging.

alafanechere · 2024-04-24T17:39:52Z

airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/connectors/test/steps/common.py

+        container = (
+            (
+                container.with_exec(["apt-get", "update"])
+                .with_exec(["apt-get", "install", "-y", "git", "openssh-client", "curl", "docker.io"])


But I did not mean I have a better suggestion. 😄

alafanechere · 2024-04-25T13:20:22Z

airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/test/pipeline.py

+    # If a package from `airbyte-platform-internal` is required, modify the entry in pyproject.toml to use https instead of ssh, when run in Github Actions
+    if is_ci:
+        container = container.with_exec(
+            ["sed", "-i", "-E", r"s,git@github\.com:airbytehq/airbyte-platform-internal,https://github.com/airbytehq/airbyte-platform-internal.git,", "pyproject.toml"]


Can we make the URL https:// in the checked in pyproject.toml.

If we did that poetry install wouldn't work for regression tests locally (whether running with pytest or airbyte-ci) unless the user has a PAT in their environment.

alafanechere · 2024-04-25T13:22:16Z

airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/test/pipeline.py

+        ).with_exec(
+            ["poetry", "source", "add", "--priority=supplemental", "airbyte-platform-internal-source",
+             "https://github.com/airbytehq/airbyte-platform-internal.git"]
+        ).with_exec(
+            [
+                "poetry", "config", "http-basic.airbyte-platform-internal-source", "octavia-squidington-iii",
+                pipeline_context_params["ci_github_access_token"],
+            ]
+        )


Can you explain in a comment why this is for?

alafanechere · 2024-04-25T13:24:56Z

airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/test/pipeline.py

@@ -190,6 +191,20 @@ def prepare_container_for_poe_tasks(
    # Set working directory to the poetry package directory
    container = container.with_workdir(f"/airbyte/{poetry_package_path}")

+    # If a package from `airbyte-platform-internal` is required, modify the entry in pyproject.toml to use https instead of ssh, when run in Github Actions


A simpler but hackier option would be to download the private package from the private repo outside of airbyte-ci in the GHA workflow and edit the pyproject.toml to point to a local path instead of using a git URL.

The best approach would be to host the private packages in a private PyPi repo.

I'll look into moving it into the GHA when I'm wrapping up the GHA-specific PR.

Agree re hosting the private package, but it didn't seem worth it for just this one thing.

clnoll · 2024-04-25T19:50:23Z

/format-fix

Format-fix job started... Check job output.

❌ Job failed.

sentry-io · 2024-04-25T22:16:52Z

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

‼️ QueryError: host directory /home/runner/.config/gcloud: rpc error: code = Unknown desc = resolve : lstat /hom... dagger.client._core in execute View Issue
‼️ ExceptionGroup: 2 exceptions were raised in the task group: anyio._backends._asyncio in __aexit__ View Issue
‼️ ExceptionGroup: 2 exceptions were raised in the task group: anyio._backends._asyncio in __aexit__ View Issue
‼️ QueryError: host directory /home/runner/.config/gcloud: rpc error: code = Unknown desc = resolve : lstat /hom... pipelines.airbyte_ci.connectors.test.steps.comm... View Issue
‼️ QueryError: lstat /tmp/regression_tests_artifacts/session_8886483582/report.html: no such file or directory pipelines.airbyte_ci.connectors.test.steps.comm... View Issue

_{Did you find this useful? React with a 👍 or 👎}

natikgadzhi · 2024-04-26T22:00:37Z

airbyte-ci/connectors/live-tests/pyproject.toml

+[tool.airbyte_ci]
+optional_poetry_groups = ["dev"]
+poe_tasks = []
+required_environment_variables = ["DOCKER_HUB_USERNAME", "DOCKER_HUB_PASSWORD"]


Can you tell me more about how we are using those credentials in live-tests? Are we using any private images today, and/or publishing? Are we logging in to dockerhub in the flow, and do we have to do it? /cc @alafanechere

natikgadzhi · 2024-04-26T22:01:27Z

airbyte-ci/connectors/live-tests/src/live_tests/cli.py

@@ -1,4 +1,5 @@
 # Copyright (c) 2023 Airbyte, Inc., all rights reserved.
+from __future__ import annotations


What version of Python are we running this on, mainly? Seemingly that is obsolete in 3.10, right?

clnoll requested a review from a team as a code owner April 19, 2024 18:34

clnoll requested a review from alafanechere April 19, 2024 18:34

clnoll force-pushed the catherine/live-tests-with-airbyte-ci branch from 80b808d to df0ed5b Compare April 23, 2024 19:48

clnoll commented Apr 23, 2024

View reviewed changes

alafanechere requested changes Apr 24, 2024

View reviewed changes

clnoll force-pushed the catherine/live-tests-with-airbyte-ci branch from bd1287b to d57c2dd Compare April 24, 2024 17:29

clnoll requested a review from alafanechere April 24, 2024 17:30

alafanechere approved these changes Apr 24, 2024

View reviewed changes

clnoll force-pushed the catherine/live-tests-with-airbyte-ci branch 3 times, most recently from 1ec9282 to 51bcc33 Compare April 25, 2024 12:22

alafanechere reviewed Apr 25, 2024

View reviewed changes

clnoll force-pushed the catherine/live-tests-with-airbyte-ci branch from 51bcc33 to d819889 Compare April 25, 2024 13:45

Regression tests: run locally with airbyte-ci

86cace2

clnoll force-pushed the catherine/live-tests-with-airbyte-ci branch 7 times, most recently from 853f9ca to 363cbb6 Compare April 25, 2024 16:48

update poetry.lock

fe882c5

clnoll force-pushed the catherine/live-tests-with-airbyte-ci branch from 363cbb6 to fe882c5 Compare April 25, 2024 17:13

poe

e54033c

clnoll added 2 commits April 25, 2024 13:41

airbyte-ci format

4b4d243

remove some poe checks that seemed to be conflicting

c37aad4

clnoll force-pushed the catherine/live-tests-with-airbyte-ci branch from 892a88c to c37aad4 Compare April 25, 2024 18:28

clnoll added 5 commits April 25, 2024 14:35

get rid of ruff linting

b3c5319

do ruff formatting

24b3a73

ruff formatting

3dbb503

add tool.airbyte_ci

144ad2e

add info to tool.airbyte_ci

ca259ef

clnoll added 5 commits April 25, 2024 16:01

add back in poe tasks

6cc5172

formatting

3605089

remove poe tasks

8b93e5f

Remove all poe tasks

4e9475c

Temporarily disable poe lint for airbyte-ci packages

7c2b897

clnoll force-pushed the catherine/live-tests-with-airbyte-ci branch from e5e899f to 7c2b897 Compare April 25, 2024 21:41

clnoll merged commit 7bd0324 into master Apr 25, 2024
35 checks passed

clnoll deleted the catherine/live-tests-with-airbyte-ci branch April 25, 2024 21:59

clnoll mentioned this pull request Apr 26, 2024

Temporarily remove regression test step #37622

Merged

natikgadzhi reviewed Apr 26, 2024

View reviewed changes

		start_timestamp = int(time.time())
		start_timestamp = int(config.getoption("--start-timestamp") or time.time())

		await container.directory(regression_tests_artifacts_dir).export(regression_tests_artifacts_dir)
		path_to_report = f"{regression_tests_artifacts_dir}/session_{int(start_timestamp)}/report.html"

		@@ -1,4 +1,5 @@
		# Copyright (c) 2023 Airbyte, Inc., all rights reserved.
		from __future__ import annotations

Regression tests: run with airbyte-ci #37440

Regression tests: run with airbyte-ci #37440

Conversation

clnoll commented Apr 19, 2024

vercel bot commented Apr 19, 2024 • edited Loading

alafanechere commented Apr 23, 2024

clnoll commented Apr 23, 2024

alafanechere commented Apr 23, 2024 • edited Loading

clnoll commented Apr 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alafanechere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alafanechere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clnoll commented Apr 25, 2024 • edited by github-actions bot Loading

sentry-io bot commented Apr 25, 2024 • edited Loading

Suspect Issues

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Regression tests: run with `airbyte-ci` #37440

Regression tests: run with `airbyte-ci` #37440

vercel bot commented Apr 19, 2024 •

edited

Loading

alafanechere commented Apr 23, 2024 •

edited

Loading

clnoll commented Apr 25, 2024 •

edited by github-actions bot

Loading

sentry-io bot commented Apr 25, 2024 •

edited

Loading