[Integration] Fixes for integration tests #3821

jsliacan · 2023-09-05T21:51:27Z

Fixes: Issue #3771

Related to Issue #3796

cfergeau · 2023-09-07T14:24:45Z

test/integration/proxy_test.go

-			Expect(RunCRCExpectSuccess("config", "set", "network-mode", networkMode), ContainSubstring("Network mode"))
+			if runtime.GOOS == "linux" {
+				Expect(RunCRCExpectSuccess("config", "set", "network-mode", networkMode), ContainSubstring("Network mode"))
+			}


I'd argue that it's not very useful to test a non-default network mode, as this is not what most users will experience. Are there some issues if we run with the default network mode on linux?

@jsliacan WDYS?

The reason why it was this way was mostly convenience - at the time when I wrote that test, you could set network mode on all platforms, so no extra code needed for linux. Also, linux system mode is subject to the e2e proxy test (which is only for linux). So this seemed to cover another (albeit less useful) case of user mode on linux.

That said, I see how it's a non-default option and thus might not be worth testing at this point. If you say that that's the case I'm happy to parametrize the proxy address by platform and use system mode on linux.

If you say that that's the case I'm happy to parametrize the proxy address by platform and use system mode on linux.

Ah, if this makes the proxy test/code simpler, this can be a reason for using user mode networking on linux. Let's keep this for now to try to fix integration tests.

With system networking, it's

crc/test/extended/util/proxy.go

Line 68 in 85cc718

addr := flag.String("127.0.0.1", ":8888", "proxy listen address") // using network-mode=user

which would need to be changed to something reachable by the VM?

NB: should have looked at your recent changes before commenting ^^

I'm surprised this works with user-mode networking though if you don't' set * host-network-access Allow TCP/IP connections from the CRC VM to services running on the host (true/false, default: false)

gbraad

overall OK. moving to a serial execution... Does this add to the total runtime?

the networkmode is an open question

test/integration/podman_test.go

cfergeau

Really not clear why the tests would still be failing :(
But looks good to me, ideally the last commit would be squashed to the right place.

jsliacan · 2023-10-02T18:07:31Z

/retest

openshift-merge-robot · 2023-10-03T09:19:59Z

@jsliacan: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-crc	`1ed1474`	link	true	`/test e2e-crc`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

cfergeau

2 minor comments, otherwise looks good!

cfergeau · 2023-10-04T12:55:36Z

test/extended/crc/cmd/cmd.go

@@ -140,11 +140,14 @@ func CheckCRCStatus(state string) error {
 		expression = `.*CRC VM:.*\s.*: .*Stopped.*`
 	}

-	err := util.ExecuteCommand(CRC("status").ToString())
+	cmd := exec.Command("crc", "status")
+	out, err := cmd.Output()


Why do we want to avoid this?

Because I need to explicitly get the stdout to check what was in it, and util.ExecuteCommand does not return that and isn't easily modifiable to do so (and even if it was, it's used in many many places which would need changing too). So going around it like above seemed better. I hope I am not overlooking something here, but that was the reasoning.

looking at util/shell.go code, CommandReturnShouldMatch("stdout" calls CompareExpectedWithActualContains(expected, shell.GetLastCmdOutput("stdout")) which is supposed to get the command stdout output. Not clear why it's not working for you :-/

I suggest keeping this one out of the PR so that we can merge it, and to address issues related to this in a follow-up PR.

In integration tests, we do not use the shell construct (to simulate a user interacting with crc in one shell session). In e2e, we do (initialized here):

err := util.StartHostShellInstance(testWithShell)

We had issues with running crc setup (with all the progress bars etc.) in e2e tests and had to move the execution of that command out of the general shell. So with integration, we didn't use the shell construct at all.

Given that we do not have an instance of ShellInstance available, functions which rely on it won't work as expected. Therefore, instead of retrieving the stdout content from the shell variable that would contain it if it existed, we need to pass it between command execution (crc status) and output analysis (does it contain Running?) by hand. And that's what this commit does. Without it, WaitForClusterInState won't have any effect (except for wasting 10mins).

So if we omit this change from this PR, integration tests will keep failing, albeit with a different error than initially when we started out.

This would be a nice explanation to have in the commit log ;)

CheckCRCStatus is used by both e2e and integration tests, is it ok to remove the use of shell for e2e?
And what about all the users of ExecuteCommand() in cmd.go? They also rely on shell.go, is this going to cause issues?

If this change is good enough to fix integration tests without breaking e2e, we can go with it, but some follow-up cleanups would be very useful imo (a clean separation between helpers using exec.Command, and helpers using shell.go). The former would only be used in integration tests, and the latter in e2e.

This would be a nice explanation to have in the commit log ;)

Will add it there.

CheckCRCStatus is used by both e2e and integration tests, is it ok to remove the use of shell for e2e?

It is ok in the sense that it'll work. However, it'll dent the effort to simulate single-session user interaction with CRC.

And what about all the users of ExecuteCommand() in cmd.go? They also rely on shell.go, is this going to cause issues?

As long as you don't need to retrieve anything aside from the error, it will do the job the same way.

some follow-up cleanups would be very useful imo

I don't know the extent of this. It could have further consequences -- maybe it makes less sense to share the code between e2e and integration then. I opened an issue to track this: #3859

maybe it makes less sense to share the code between e2e and integration then

Maybe some go interface can be defined and used in the shared code. The interface would either make use of the shell code to run the command/get stdout/..., or would directly run the command for the integration tests.

test/extended/util/proxy.go

cfergeau · 2023-10-04T18:39:05Z

test/integration/podman_test.go

 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 )

-var _ = Describe("podman preset", Serial, Label("podman-preset"), func() {
+var _ = Describe("podman preset", Serial, Ordered, Label("podman-preset"), func() {


Is it intentional that the Ordered addition is in this integration: cleanup to AfterAll and BeforeSuite commit and not in integration: run ginkgo nodes serially and in ordered fashion? If it's not intentional, I've already split it in https://github.com/cfergeau/crc/commits/ginkgo-issue

jsliacan · 2023-10-05T09:30:02Z

I forgot that I need to unset HTTPS_PROXY and HTTP_PROXY env variables that are implicitly set during the test.

cfergeau

I do not see 497c1d0#r1346340794 in the commit log for " tests: avoid using shell struct in CheckCRCStatus" but apart from this, ACK.

The outer Describe block needs to be Ordered to be able to use AfterAll inside it. Cleanup in BeforeSuite and AfterAll functions should take care of clean environment before and after each testcase.

In e2e we use WaitForClusterInState after `crc start` finishes to give time to the cluster to stabilize (all operators are reliably up). This is also needed here before we login to the cluster.

In integration tests we do not make use of ShellInstance, so in order to be able to share WaitForClusterInState between e2e and integration, we need CheckCRCStatus not to rely on ShellInstance. See also this comment: crc-org#3821 (comment).

jsliacan · 2023-10-10T11:14:16Z

Last changes include git reset onto @cfergeau 's branch with improved commits, additional unset HTTP(S)_PROXY in AfterAll, and updated commit message to explain why we're changing CheckCRCStatus to avoid using ShellInstance.

cfergeau · 2023-10-12T13:45:20Z

test/integration/podman_test.go

@@ -50,7 +50,7 @@ var _ = Describe("podman preset", Serial, Ordered, Label("podman-preset"), func(
 				unexpandedPath := filepath.Join(userHomeDir, ".crc/bin/oc;${PATH}")
 				path = os.ExpandEnv(unexpandedPath)
 				csshk = filepath.Join(userHomeDir, ".crc/machines/crc/id_ecdsa")
-				dh = "npipe:////./pipe/rc-podman"
+				dh = "npipe:////./pipe/crc-podman"


This also happens in the E2E tests

True, thanks. Grepped it now and the e2e occurrence was the last one. Changed it and updated the commit too.

openshift-ci · 2023-10-12T14:26:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfergeau

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [cfergeau]

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

cfergeau · 2023-10-12T14:50:41Z

(waiting for integration-crc to run before merging it)

openshift-ci · 2023-10-12T17:37:43Z

@jsliacan: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-crc	`23adf9a`	link	true	`/test e2e-crc`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

praveenkumar · 2023-10-13T04:27:41Z

e2e-integration now succeed.

In integration tests we do not make use of ShellInstance, so in order to be able to share WaitForClusterInState between e2e and integration, we need CheckCRCStatus not to rely on ShellInstance. See also this comment: #3821 (comment).

openshift-ci bot requested review from adrianriobo and praveenkumar September 5, 2023 21:51

jsliacan added the do-not-merge/work-in-progress label Sep 5, 2023

jsliacan force-pushed the ginkgo-issue branch from 7217725 to eeba875 Compare September 6, 2023 08:43

openshift-ci bot removed the do-not-merge/work-in-progress label Sep 6, 2023

cfergeau reviewed Sep 7, 2023

View reviewed changes

openshift-merge-robot added the needs-rebase label Sep 16, 2023

gbraad reviewed Sep 18, 2023

View reviewed changes

jsliacan force-pushed the ginkgo-issue branch from eeba875 to 20f95c2 Compare September 18, 2023 07:12

openshift-merge-robot removed the needs-rebase label Sep 18, 2023

jsliacan force-pushed the ginkgo-issue branch 2 times, most recently from 2f86ee5 to fb8382e Compare September 20, 2023 07:50

cfergeau reviewed Sep 29, 2023

View reviewed changes

test/integration/podman_test.go Show resolved Hide resolved

cfergeau approved these changes Sep 29, 2023

View reviewed changes

openshift-ci bot assigned cfergeau Sep 29, 2023

openshift-ci bot added lgtm approved labels Sep 29, 2023

jsliacan force-pushed the ginkgo-issue branch from fb8382e to e5a8ab9 Compare October 2, 2023 12:50

openshift-ci bot removed the lgtm label Oct 2, 2023

jsliacan force-pushed the ginkgo-issue branch from e5a8ab9 to b72344a Compare October 2, 2023 12:52

jsliacan force-pushed the ginkgo-issue branch from b72344a to 1ed1474 Compare October 3, 2023 06:51

cfergeau reviewed Oct 4, 2023

View reviewed changes

jsliacan force-pushed the ginkgo-issue branch from 1ed1474 to b3fb458 Compare October 4, 2023 14:33

cfergeau reviewed Oct 4, 2023

View reviewed changes

jsliacan added the do-not-merge/hold label Oct 5, 2023

jsliacan mentioned this pull request Oct 5, 2023

[QE] Separate test code that uses ShellInstance from that which does not #3859

Open

cfergeau approved these changes Oct 9, 2023

View reviewed changes

jsliacan force-pushed the ginkgo-issue branch from f6374bf to 2e47f70 Compare October 9, 2023 19:49

jsliacan added 8 commits October 10, 2023 10:00

integration: run ginkgo nodes serially and in ordered fashion

ee76ec0

integration: refactor top-level testsuite info and actions

251b170

integration: cleanup to AfterAll and BeforeSuite

cdb9fb7

The outer Describe block needs to be Ordered to be able to use AfterAll inside it. Cleanup in BeforeSuite and AfterAll functions should take care of clean environment before and after each testcase.

integration: use default network mode for linux

b0b6640

tests: move util functions to extended utils

9377da8

integration: wait for cluster in state

47d3891

In e2e we use WaitForClusterInState after `crc start` finishes to give time to the cluster to stabilize (all operators are reliably up). This is also needed here before we login to the cluster.

integration: extend timeout

c962adb

jsliacan force-pushed the ginkgo-issue branch from 2e47f70 to f50b81c Compare October 10, 2023 11:02

jsliacan removed the do-not-merge/hold label Oct 10, 2023

jsliacan force-pushed the ginkgo-issue branch from 42edf70 to 840e690 Compare October 10, 2023 20:43

integration: set CRCHome var before using it

0a47db1

jsliacan force-pushed the ginkgo-issue branch from 840e690 to 0a47db1 Compare October 11, 2023 10:33

cfergeau approved these changes Oct 12, 2023

View reviewed changes

openshift-ci bot added the lgtm label Oct 12, 2023

tests: remove typo

23adf9a

jsliacan force-pushed the ginkgo-issue branch from 93866b7 to 23adf9a Compare October 12, 2023 14:11

openshift-ci bot removed the lgtm label Oct 12, 2023

cfergeau approved these changes Oct 12, 2023

View reviewed changes

openshift-ci bot added the lgtm label Oct 12, 2023

praveenkumar merged commit 65e3a58 into crc-org:main Oct 13, 2023
13 checks passed

This was referenced Oct 19, 2023

[BUG] [Integration] CRC not starting behind on-the-host proxy without host-network-access #3880

Closed

[integration-test] disk-resize test failing on openshift CI #3771

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Integration] Fixes for integration tests #3821

[Integration] Fixes for integration tests #3821

jsliacan commented Sep 5, 2023 •

edited

Loading

cfergeau Sep 7, 2023

gbraad Sep 18, 2023

jsliacan Sep 18, 2023

cfergeau Sep 29, 2023

cfergeau Sep 29, 2023 •

edited

Loading

cfergeau Sep 29, 2023

gbraad left a comment

cfergeau left a comment

jsliacan commented Oct 2, 2023

openshift-merge-robot commented Oct 3, 2023

cfergeau left a comment

cfergeau Oct 4, 2023

jsliacan Oct 4, 2023

cfergeau Oct 4, 2023

cfergeau Oct 4, 2023

jsliacan Oct 4, 2023

cfergeau Oct 5, 2023

jsliacan Oct 5, 2023

cfergeau Oct 9, 2023

cfergeau Oct 4, 2023

jsliacan commented Oct 5, 2023 •

edited

Loading

cfergeau left a comment

jsliacan commented Oct 10, 2023

cfergeau Oct 12, 2023

jsliacan Oct 12, 2023

openshift-ci bot commented Oct 12, 2023

cfergeau commented Oct 12, 2023

openshift-ci bot commented Oct 12, 2023

praveenkumar commented Oct 13, 2023

[Integration] Fixes for integration tests #3821

[Integration] Fixes for integration tests #3821

Conversation

jsliacan commented Sep 5, 2023 • 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

cfergeau Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbraad left a comment

Choose a reason for hiding this comment

cfergeau left a comment

Choose a reason for hiding this comment

jsliacan commented Oct 2, 2023

openshift-merge-robot commented Oct 3, 2023

cfergeau 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

jsliacan commented Oct 5, 2023 • edited Loading

cfergeau left a comment

Choose a reason for hiding this comment

jsliacan commented Oct 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 12, 2023

cfergeau commented Oct 12, 2023

openshift-ci bot commented Oct 12, 2023

praveenkumar commented Oct 13, 2023

jsliacan commented Sep 5, 2023 •

edited

Loading

cfergeau Sep 29, 2023 •

edited

Loading

jsliacan commented Oct 5, 2023 •

edited

Loading