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.

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

[Testing] python presubmit tests failing 2021.3.1 #5209

Closed
Bobgy opened this issue Mar 1, 2021 · 6 comments · Fixed by #5211 or #5220
Closed

[Testing] python presubmit tests failing 2021.3.1 #5209

Bobgy opened this issue Mar 1, 2021 · 6 comments · Fixed by #5211 or #5220
Assignees

Comments

@Bobgy
Copy link
Contributor

Bobgy commented Mar 1, 2021

https://oss-prow.knative.dev/view/gs/oss-prow/pr-logs/pull/kubeflow_pipelines/5196/kubeflow-pipelines-sdk-python36/1365228134722441216

/assign @chensun

@Bobgy
Copy link
Contributor Author

Bobgy commented Mar 1, 2021

Found in #5196

Why did it require docker?

@chensun
Copy link
Member

chensun commented Mar 1, 2021

Why did it require docker?

The test log seems to be messed up. The docker requirement is from another test which somehow didn't shown as "FAILED". That test explicitly asks for testing with docker:

execution_mode=LocalClient.ExecutionMode(mode="docker"),

Not sure if it was this error that messed up the subsequent tests.

Did we recently remove docker from the CI test environment?

@Bobgy
Copy link
Contributor Author

Bobgy commented Mar 1, 2021

I don't think we ever had docker there, in the CI test env, tests are run inside a Pod, so there shouldn't be docker there.

The test was introduced in #4983, curious why it didn't fail originally

@chensun
Copy link
Member

chensun commented Mar 1, 2021

I don't think we ever had docker there, in the CI test env, tests are run inside a Pod, so there shouldn't be docker there.

The test was introduced in #4983, curious why it didn't fail originally

Then I was probably mistaken -- the missing docker error likely didn't fail the test. Although I think we should eliminate such error anyway. Besides, a UT shouldn't have dependencies on docker, best practice should be using mock here.

That left one possibility: the other test was flaky.

Reopen this issue as P1, I'll spend some time debugging the flaky test some time this week.

@chensun chensun reopened this Mar 1, 2021
@Ark-kun
Copy link
Contributor

Ark-kun commented Mar 1, 2021

On first glance the test_execution_mode_exclude_op test seems to have been working as intended.
It tried to explicitly use docker and failed. Then tried excluding the image (so that it does not run on docker) and that succeeds.
The tests were passing here: https://oss-prow.knative.dev/view/gs/oss-prow/pr-logs/pull/kubeflow_pipelines/4983/kubeflow-pipelines-sdk-python39/1364431670366703616

@chensun
Copy link
Member

chensun commented Mar 1, 2021

On first glance the test_execution_mode_exclude_op test seems to have been working as intended.
It tried to explicitly use docker and failed. Then tried excluding the image (so that it does not run on docker) and that succeeds.
The tests were passing here: https://oss-prow.knative.dev/view/gs/oss-prow/pr-logs/pull/kubeflow_pipelines/4983/kubeflow-pipelines-sdk-python39/1364431670366703616

Right, I realized that. Missing docker didn't fail the test, it yields the same expected behavior though through a different route: "docker not found" vs "image not found".
That said, I think we should use mock to avoid actually calling docker binary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment