chore(tests): added makefiles for testing workflow tests locally #11509

chahatsagarmain · 2025-01-11T23:25:12Z

Description of your changes:
Issue #11494
Added makefiles for tests and setup each test locally .

Checklist:

You have signed off your commits
The title for your pull request (PR) should follow our title convention. Learn more about the pull request title convention used in this repository.

google-oss-prow · 2025-01-11T23:25:23Z

Hi @chahatsagarmain. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

DharmitD

/lgtm

Thanks!

DharmitD · 2025-01-12T14:25:08Z

/ok-to-test

hbelmiro · 2025-01-13T17:20:26Z

@anishasthana you may like to review this.

jgarciao · 2025-01-16T16:31:09Z

Could you document how to launch the tests in https://github.com/kubeflow/pipelines/blob/master/test/README.md ?

hbelmiro · 2025-01-16T18:21:26Z

@chahatsagarmain I added one more item to the issue's acceptance criteria.

Workflows need to be updated to use the new make targets

mahdikhashan · 2025-01-20T11:29:03Z

hello @chahatsagarmain , following up with our chat on slack, would you please make sure that this command (make test-requirements) is addressed in the makefile correctly?

related commit in parallel pr:

ce7f54c

it should point to the test-requirements.sh in the dir .github/resources/scripts/.

thanks for your collaboration in advance.

cc: @hbelmiro

hbelmiro

@chahatsagarmain please don't forget the actions.

hbelmiro · 2025-01-20T12:19:28Z

.github/workflows/presubmit-backend.yml

Please add the missing .github/workflows/presubmit-backend.yml to on.pull_request.paths.

hbelmiro · 2025-01-20T12:20:58Z

.github/workflows/sdk-component-yaml.yml

@@ -53,4 +53,4 @@ jobs:
        run: pip install -r ./test/sdk-execution-tests/requirements.txt

      - name: Run component YAML tests
-        run: ./test/presubmit-component-yaml.sh
+        run: make sdk-component-yaml


You missed the setup for this one.

hbelmiro · 2025-01-20T12:22:01Z

.github/workflows/sdk-execution.yml

-          export KFP_ENDPOINT="http://localhost:8888"
-          export TIMEOUT_SECONDS=2700
-          pytest ./test/sdk-execution-tests/sdk_execution_tests.py --asyncio-task-timeout $TIMEOUT_SECONDS
+          make sdk-execution


You missed the setup for this one.

hbelmiro · 2025-01-20T12:22:50Z

.github/workflows/sdk-yapf.yml

@@ -33,4 +33,4 @@ jobs:
      run: pip install yapf

    - name: Run YAPF SDK Tests
-      run: ./test/presubmit-yapf-sdk.sh
+      run: make sdk-yapf


You missed the setup for this one.

hbelmiro · 2025-01-20T12:24:48Z

Makefile

Please rename them to have test- in the beginning of their names so it will be easier for contributors to find them.
For example, backend-test-flip-coin -> test-backend-flip-coin.

hbelmiro · 2025-01-20T12:29:03Z

/ok-to-test

github-actions · 2025-01-20T12:29:52Z

Approvals successfully granted for pending runs.

hbelmiro · 2025-01-20T12:41:32Z

hello @chahatsagarmain , following up with our chat on slack, would you please make sure that this command (make test-requirements) is addressed in the makefile correctly?

related commit in parallel pr:

ce7f54c

it should point to the test-requirements.sh in the dir .github/resources/scripts/.

thanks for your collaboration in advance.

cc: @hbelmiro

@mahdikhashan as per #11512 (comment), you need to add the new target in your PR, otherwise it won't pass the CI and can't get merged.

hbelmiro · 2025-01-20T18:59:17Z

/ok-to-test

github-actions · 2025-01-20T18:59:53Z

Approvals successfully granted for pending runs.

hbelmiro · 2025-01-21T14:59:54Z

/ok-to-test

github-actions · 2025-01-21T15:10:55Z

Approvals successfully granted for pending runs.

hbelmiro · 2025-01-23T19:36:57Z

/ok-to-test

github-actions · 2025-01-23T19:37:32Z

Approvals successfully granted for pending runs.

hbelmiro · 2025-02-20T17:10:54Z

@chahatsagarmain can you please rebase?

chahatsagarmain · 2025-02-20T17:16:08Z

@hbelmiro I think its already up to date with latest master commit

hbelmiro

/lgtm

google-oss-prow · 2025-02-20T18:53:10Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: hbelmiro
Once this PR has been reviewed and has the lgtm label, please assign zijianjoy for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

HumairAK · 2025-02-21T16:21:14Z

Makefile.setup.mk

+.PHONY: create-kind-cluster
+create-kind-cluster:
+	kind create cluster --name kfp --image kindest/node:v1.29.2 --wait 5m && \
+	kubectl version --client --short


Where is this used? Or is this meant for local set up? If the latter it might be worth adding this to the readme

HumairAK · 2025-02-21T16:21:59Z

test/README.md


+4.  Install the necessary dependencies for the test by running the command `make setup-{test_name}`. The steps for setting up dependencies are defined in the `Makefile.setup.mk` file.
+
+5.  Execute the test using command `make {test_name}` , all the test that can be run using Make commands are mentioned in `Makefile`. 
+


aren't these meant to replace the "## Running Tests Locally with Kind" section entirely? or at the least the user is likely going to follow one approach or the other, but not both, so we can document it such that we tell the user "you can use the make by doing xyz steps, or you can run it manually by doing abc steps"

I think we can replace the "## Running Tests Locally with Kind" section entirely.

HumairAK · 2025-02-21T16:58:26Z

Makefile.setup.mk

+	sudo apt-get update && \
+	sudo apt-get install protobuf-compiler -y && \


putting apt-get in a make file here is not a good idea, we should keep this scoped to the environment, this should be handled in the github action (preferably as it's own step), and the user should install this themselves (and we should provide docs as pre-requisites)

for example, if the intent of the pr is to allow users to run these locally, what if they are not using a debian os? we should try to be as platform agnostic as we can (this applies to any make make command here that's using apt-get)

@HumairAK The PR's intent is to allow contributors to find all tests in one place using make commands.

putting apt-get in a make file here is not a good idea, we should keep this scoped to the environment, this should be handled in the github action (preferably as it's own step), and the user should install this themselves (and we should provide docs as pre-requisites)

for example, if the intent of the pr is to allow users to run these locally, what if they are not using a debian os? we should try to be as platform agnostic as we can (this applies to any make make command here that's using apt-get)

I agree 100%. At the same time, keeping this in the GitHub action kind of goes against the reason for this PR.
How about leaving it in the make file for now, but adding a verification to check if it's running on debian before running any command? We could add compatibility with other SOs, like Fedora and macOS, in a follow-up PR/issue.

is_debian() { # Check via /etc/os-release (most reliable) if [ -f /etc/os-release ]; then if grep -q 'ID=debian' /etc/os-release; then return 0 fi fi # Fallback check for /etc/debian_version if [ -f /etc/debian_version ]; then return 0 fi # If neither check passed return 1 }

We shouldn't be installing system wide packages in e2e makefile targets, pip install is fine since it's scoped to venv, but apt-get installs it in the user's OS. We want this to be an active action, and not a passive one. I'm fine with providing a make target like "install-preq-requisites" and have this be a separate step before invoking the rest of the test suite, but not as part of a full e2e single command.

Yeah, one of the issue's acceptance criteria is exactly that:

make commands for running tests should not install any tools. In case of having a tool as a pre-requirement, we can have a specific make command for that.

It's what @chahatsagarmain did here if I'm not mistaken.

HumairAK · 2025-02-21T17:08:32Z

Makefile.setup.mk

+	python3 -m venv .venv && \
+	source .venv/bin/activate


various tests are installing a virtual environment with python packages in the user environment via pip,
what is the behavior if they re-run the tests?

it would be good if tests re-run dont' have to always re-install the packages and re-use the env, we should also document that some of the make commands will install a venv and packages

I'm thinking it would be good to have clean up commands in those scenarios but because there are so many different tests each with unique cases, I'm not sure how feasible that would be

various tests are installing a virtual environment with python packages in the user environment via pip,
what is the behavior if they re-run the tests?

it would be good if tests re-run dont' have to always re-install the packages and re-use the env, we should also document that some of the make commands will install a venv and packages

I think that the installation of packages should be in the setup make commands.

I'm thinking it would be good to have clean up commands in those scenarios but because there are so many different tests each with unique cases, I'm not sure how feasible that would be

As we have setup make commands, we could also have tear-down commands. In that case, we could print a message at the end of the test run warning contributors that they need to run the tear-down command.
How about adding it in a follow-up PR/issue? This is a big PR and we'll likely have other improvements in the CI in the next few days. Rebasing this PR makes the review difficult/error-prone.

HumairAK · 2025-02-21T17:09:15Z

Makefile.setup.mk

+	python3 -m pip install api/v2alpha1/python && \
+	pip install components/google-cloud && \
+	pip install $(shell grep 'pytest==' sdk/python/requirements-dev.txt) && \
+	pytest ./test/gcpc-tests/run_all_gcpc_modules.py


this looks like it shouldn't be here, as it's only a setup target and the same command is ran in test-grpc-modulestest-grpc-modules

HumairAK · 2025-02-21T17:29:13Z

Makefile.setup.mk

+	pytest ./test/gcpc-tests/run_all_gcpc_modules.py
+
+.PHONY: setup-kfp-kubernetes-execution-tests
+setup-kfp-kubernetes-execution-tests:


I don't see this used in any github action, was there a reason for this?

HumairAK · 2025-02-21T17:34:50Z

.github/workflows/sdk-execution.yml

      - name: Install requirements
-        run: pip install -r ./test/sdk-execution-tests/requirements.txt
+        run: make setup-sdk-execution


this looks like we are deploying KFP twice [1], [2] ?

google-oss-prow · 2025-02-27T17:51:57Z

New changes are detected. LGTM label has been removed.

parent 7bb0c44 author chahatsagarmain <chahatsagar2003@gmail.com> 1740067717 +0530 committer chahatsagarmain <chahatsagar2003@gmail.com> 1740068190 +0530 parent 7bb0c44 author chahatsagarmain <chahatsagar2003@gmail.com> 1740067717 +0530 committer chahatsagarmain <chahatsagar2003@gmail.com> 1740068098 +0530 make commands for local testing and ci refactor Signed-off-by: chahatsagarmain <chahatsagar2003@gmail.com> chore: use local proto go packages for ci (kubeflow#11629) Signed-off-by: Humair Khan <HumairAK@users.noreply.github.com> fix(backend): upgrade go version to 1.22.12 to fix CVE-2024-45336 (kubeflow#11631) Signed-off-by: Daniel Dowler <12484302+dandawg@users.noreply.github.com> fix(backend) fix run retry for argo (kubeflow#11585) Signed-off-by: arpechenin <arpechenin@avito.ru> fix(frontend): restrict file explorer to show only .yaml, .yml, .zip, and .tar.gz files (kubeflow#11623) * fix(frontend): restrict file explorer to show only .yaml, .yml, .zip, and .tar.gz files Signed-off-by: muzzlol <muzxmmilkhxn@gmail.com> * test(frontend): update NewPipelineVersion snapshot for file filters Signed-off-by: muzzlol <muzxmmilkhxn@gmail.com> --------- Signed-off-by: muzzlol <muzxmmilkhxn@gmail.com> fix(backend): fixes DAG status update to reflect completion of all tasks (kubeflow#11651) Signed-off-by: VaniHaripriya <vmudadla@redhat.com> chore(test): Increased timeout in test (kubeflow#11658) Signed-off-by: Helber Belmiro <helber.belmiro@gmail.com> make commands to test locally and ci refactor Signed-off-by: chahatsagarmain <chahatsagar2003@gmail.com> add missing step Signed-off-by: chahatsagarmain <chahatsagar2003@gmail.com> reviewed changes Signed-off-by: chahatsagarmain <chahatsagar2003@gmail.com> fix eol Signed-off-by: chahatsagarmain <chahatsagar2003@gmail.com>

hbelmiro · 2025-02-27T20:39:55Z

@HumairAK @chahatsagarmain
This is a big PR that moves code around. I think it's dangerous to leave it open for too much time. I'm afraid that we may miss some commits to the master branch while rebasing this PR.
How about breaking it in several easy-to-review/merge PRs? We could turn #11494 into an epic and have one new issue for each workflow.

hbelmiro · 2025-02-28T10:14:38Z

/close

We'll proceed with individual PRs for each workflow.

google-oss-prow · 2025-02-28T10:14:43Z

@hbelmiro: Closed this PR.

In response to this:

/close

We'll proceed with individual PRs for each workflow.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

google-oss-prow bot added needs-ok-to-test size/L labels Jan 11, 2025

google-oss-prow bot requested review from HumairAK and zijianjoy January 11, 2025 23:25

DharmitD reviewed Jan 12, 2025

View reviewed changes

google-oss-prow bot assigned DharmitD Jan 12, 2025

google-oss-prow bot added the lgtm label Jan 12, 2025

google-oss-prow bot added ok-to-test and removed needs-ok-to-test labels Jan 12, 2025

google-oss-prow bot removed the lgtm label Jan 19, 2025

hbelmiro reviewed Jan 20, 2025

View reviewed changes

hbelmiro mentioned this pull request Jan 20, 2025

chore(tests): create a test to check whether requirements.txt was generated with pip-compile #11512

Closed

2 tasks

google-oss-prow bot added size/XL and removed size/L labels Jan 20, 2025

chahatsagarmain requested a review from hbelmiro January 20, 2025 18:45

google-oss-prow bot removed the lgtm label Feb 18, 2025

chahatsagarmain force-pushed the makefile branch from 9e18ef4 to c73d65d Compare February 18, 2025 20:09

google-oss-prow bot added size/XXL size/XL and removed size/XL size/XXL labels Feb 18, 2025

chahatsagarmain force-pushed the makefile branch from 8f2a238 to cfec71c Compare February 18, 2025 20:24

chahatsagarmain requested a review from hbelmiro February 18, 2025 20:25

chahatsagarmain force-pushed the makefile branch from cfec71c to 2eb3d40 Compare February 20, 2025 16:08

google-oss-prow bot added size/XXL and removed size/XL labels Feb 20, 2025

chahatsagarmain force-pushed the makefile branch from 2eb3d40 to a85e064 Compare February 20, 2025 16:39

google-oss-prow bot added size/XL and removed size/XXL labels Feb 20, 2025

chahatsagarmain force-pushed the makefile branch from a85e064 to a4a5abc Compare February 20, 2025 17:48

hbelmiro approved these changes Feb 20, 2025

View reviewed changes

google-oss-prow bot added the lgtm label Feb 20, 2025

HumairAK requested changes Feb 21, 2025

View reviewed changes

chahatsagarmain force-pushed the makefile branch from a4a5abc to e379fc7 Compare February 27, 2025 17:51

google-oss-prow bot removed the lgtm label Feb 27, 2025

chahatsagarmain force-pushed the makefile branch from e379fc7 to 09c3091 Compare February 27, 2025 18:00

google-oss-prow bot closed this Feb 28, 2025


		4. Install the necessary dependencies for the test by running the command `make setup-{test_name}`. The steps for setting up dependencies are defined in the `Makefile.setup.mk` file.

		5. Execute the test using command `make {test_name}` , all the test that can be run using Make commands are mentioned in `Makefile`.

		sudo apt-get update && \
		sudo apt-get install protobuf-compiler -y && \

chore(tests): added makefiles for testing workflow tests locally #11509

chore(tests): added makefiles for testing workflow tests locally #11509

Conversation

chahatsagarmain commented Jan 11, 2025 • edited Loading

google-oss-prow bot commented Jan 11, 2025

DharmitD left a comment

Choose a reason for hiding this comment

DharmitD commented Jan 12, 2025

hbelmiro commented Jan 13, 2025

jgarciao commented Jan 16, 2025

hbelmiro commented Jan 16, 2025

mahdikhashan commented Jan 20, 2025

hbelmiro 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

hbelmiro commented Jan 20, 2025

github-actions bot commented Jan 20, 2025

hbelmiro commented Jan 20, 2025

hbelmiro commented Jan 20, 2025

github-actions bot commented Jan 20, 2025

hbelmiro commented Jan 21, 2025

github-actions bot commented Jan 21, 2025

hbelmiro commented Jan 23, 2025

github-actions bot commented Jan 23, 2025

hbelmiro commented Feb 20, 2025

chahatsagarmain commented Feb 20, 2025

hbelmiro left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Feb 20, 2025

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

hbelmiro Feb 22, 2025 • edited Loading

Choose a reason for hiding this comment

HumairAK Feb 24, 2025 • edited Loading

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

google-oss-prow bot commented Feb 27, 2025

hbelmiro commented Feb 27, 2025

hbelmiro commented Feb 28, 2025

google-oss-prow bot commented Feb 28, 2025

chahatsagarmain commented Jan 11, 2025 •

edited

Loading

hbelmiro Feb 22, 2025 •

edited

Loading

HumairAK Feb 24, 2025 •

edited

Loading