feat: Initial E2E framework and simple tests #211

jopit · 2024-10-30T18:20:02Z

Initial E2E framework and simple tests

Resolves issue #137

jannfis

LGTM

jgwest

Looks great, very nice, thanks @jopit! I added some minor comments I noticed while reading through the code, let me know what you think.

I had a couple of questions but they are more general project questions for everyone, so I will loop in Jann et al.

One other question from me:

Any changes in 'argocd-agent/test/e2e2' that you would suggest we may want to port back to 'argocd-agent/hack/demo-env'?

For example, extra timeout logic in 'setup-vcluster-env.sh' looked useful for both
Removal of .data from Secrets defined in argocd-secret.yaml

jgwest · 2024-11-02T12:12:37Z

Makefile

+setup-e2e2:
+	test/e2e2/test-env/setup-vcluster-env.sh create
+
+.PHONY: start-argocd-agent


Re: the Makefile target of start-argocd-agent. Rather than start-argocd-agent, which might imply this would start Argo agent for development purposes (similar to make start from Argo CD), WDYT about:

start-argocd-agent-for-e2e (maybe too long?)

start-e2e / start-e2e2 (shorter, this is what we use in argo cd, and in rollouts operator)

Other options?

Done. Changed it to start-e2e2

jgwest · 2024-11-02T12:26:28Z

test/e2e2/fixture/kubeclient.go

+	"k8s.io/client-go/restmapper"
+)
+
+type KubeObject interface {


Could add a comment here for KubeObject and KubeObjectList indicating these are similar to Object and ObjectList from controller-runtime, or even more generally that this package aims to provide a similar style interface to controller-runtime (which we are avoiding adding a dependency on here).

IMHO this might be useful comment to add (and presuming my assumption is true), but it's up to you!

jgwest · 2024-11-02T12:37:14Z

test/e2e2/basic_test.go

+		err := suite.ManagedAgentClient.Get(suite.Ctx, key, &app, metav1.GetOptions{})
+		return err == nil
+	}, 30*time.Second, 1*time.Second)
+


Can we check that the content (.spec field) is the same between the managed-agent and principal, as well? It's very likely, but it can't hurt to check.

jgwest · 2024-11-02T12:38:54Z

test/e2e2/basic_test.go

+		return err == nil
+	}, 30*time.Second, 1*time.Second)
+
+	// Delete the app from the principal


Let me know if you think this is scope creep from the original requirements, but before we delete the Application in the next step, can we test the update case, as well?

E.g. just making a small change to one of the fields of the Application on principal, and making sure it percolates down to managed-agent.

jgwest · 2024-11-02T12:39:48Z

test/e2e2/basic_test.go

+	// Delete the app from the autonomous-agent
+	err = suite.AutonomousAgentClient.Delete(suite.Ctx, &app, metav1.DeleteOptions{})
+	requires.NoError(err)
+


Same as above, can be useful to check .spec is in sync, and to test the update case before deleting.

jgwest · 2024-11-02T13:11:17Z

test/e2e2/sync_test.go

+		err := suite.ManagedAgentClient.Get(suite.Ctx, key, &app, metav1.GetOptions{})
+		return err == nil
+	}, 30*time.Second, 1*time.Second)
+


As mentioned elsewhere, can be useful to verify that the .Spec fields are the same in both places.

jgwest · 2024-11-02T13:13:27Z

test/e2e2/sync_test.go

+		err := suite.PrincipalClient.Get(suite.Ctx, principalKey, &app, metav1.GetOptions{})
+		return err == nil
+	}, 30*time.Second, 1*time.Second)
+


As above, couldn't hurt to verify .specs are the same.

jgwest · 2024-11-02T13:32:40Z

test/e2e2/test-env/start-agent-autonomous.sh

+    exit 1
+fi
+SCRIPTPATH="$( cd -- "$(dirname "$0")" >/dev/null 2>&1 ; pwd -P )"
+test -f cmd/agent/main.go || (echo "Script should be run from argocd-agent's root path" >&2; exit 1)


Rather than testing that the script is run from the correct place, would it be preferable to just add cd $SCRIPTPATH/../../.. before go run to ensure that this is always true?

Resolved this by making the same changes that Jann made to the demo-env scripts to allow running from anywhere in the repo tree, PR #193

jgwest · 2024-11-02T13:32:56Z

test/e2e2/test-env/start-agent-managed.sh

+    exit 1
+fi
+SCRIPTPATH="$( cd -- "$(dirname "$0")" >/dev/null 2>&1 ; pwd -P )"
+test -f cmd/agent/main.go || (echo "Script should be run from argocd-agent's root path" >&2; exit 1)


Same as automonous above, can use SCRIPTPATH if it makes sense.

Resolved this by making the same changes that Jann made to the demo-env scripts to allow running from anywhere in the repo tree, PR #193

jgwest · 2024-11-02T13:33:13Z

test/e2e2/test-env/start-principal.sh

+    exit 1
+fi
+SCRIPTPATH="$( cd -- "$(dirname "$0")" >/dev/null 2>&1 ; pwd -P )"
+test -f cmd/principal/main.go || (echo "Script should be run from argocd-agent's root path" >&2; exit 1)


Same as autonomous above, can use SCRIPTPATH if it makes sense.

Resolved this by making the same changes that Jann made to the demo-env scripts to allow running from anywhere in the repo tree, PR #193

codecov-commenter · 2024-11-05T18:39:30Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 46.83%. Comparing base (237faa7) to head (5bd3dca).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #211      +/-   ##
==========================================
- Coverage   46.89%   46.83%   -0.06%     
==========================================
  Files          57       57              
  Lines        4956     4951       -5     
==========================================
- Hits         2324     2319       -5     
  Misses       2451     2451              
  Partials      181      181

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: John Pitman <[email protected]>

jopit requested review from jannfis, jgwest and ishitasequeira as code owners October 30, 2024 18:20

jannfis approved these changes Oct 31, 2024

View reviewed changes

jgwest requested changes Nov 4, 2024

View reviewed changes

jopit force-pushed the gitops-5159-e2e-test-framework branch from f4f1592 to f70c232 Compare November 5, 2024 18:49

jopit added 3 commits November 5, 2024 13:51

feat: add e2e test framework and initial tests

6a7ce0c

Signed-off-by: John Pitman <[email protected]>

remove CI action for this branch

3b2f0ce

Signed-off-by: John Pitman <[email protected]>

updates based on PR feedback

b5afc9b

Signed-off-by: John Pitman <[email protected]>

jopit force-pushed the gitops-5159-e2e-test-framework branch 2 times, most recently from b08036b to 7c34544 Compare November 7, 2024 19:44

generate server.secretkey for the agents

5bd3dca

Signed-off-by: John Pitman <[email protected]>

jopit force-pushed the gitops-5159-e2e-test-framework branch from 7c34544 to 5bd3dca Compare November 7, 2024 19:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Initial E2E framework and simple tests #211

feat: Initial E2E framework and simple tests #211

jopit commented Oct 30, 2024 •

edited

Loading

jannfis left a comment

jgwest left a comment •

edited

Loading

jgwest Nov 2, 2024

jopit Nov 5, 2024

jgwest Nov 2, 2024

jopit Nov 5, 2024

jgwest Nov 2, 2024

jopit Nov 5, 2024

jgwest Nov 2, 2024

jopit Nov 5, 2024

jgwest Nov 2, 2024

jopit Nov 5, 2024

jgwest Nov 2, 2024

jopit Nov 5, 2024

jgwest Nov 2, 2024

jopit Nov 5, 2024

jgwest Nov 2, 2024

jopit Nov 5, 2024

jgwest Nov 2, 2024

jopit Nov 5, 2024

jgwest Nov 2, 2024

jopit Nov 5, 2024

codecov-commenter commented Nov 5, 2024 •

edited

Loading

feat: Initial E2E framework and simple tests #211

Are you sure you want to change the base?

feat: Initial E2E framework and simple tests #211

Conversation

jopit commented Oct 30, 2024 • edited Loading

jannfis left a comment

Choose a reason for hiding this comment

jgwest left a comment • 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

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

codecov-commenter commented Nov 5, 2024 • edited Loading

Codecov Report

jopit commented Oct 30, 2024 •

edited

Loading

jgwest left a comment •

edited

Loading

codecov-commenter commented Nov 5, 2024 •

edited

Loading