Start migrating e2e tests to Go. #3079

v-shenoy · 2022-05-30T06:28:18Z

Signed-off-by: Vighnesh Shenoy vshenoy@microsoft.com

Provide a description of what has been changed

Start migrating e2e tests to Go for code parity.

Sample Logs

Checklist

Commits are signed with Developer Certificate of Origin (DCO - learn more)
Tests have been added
A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
A PR is opened to update the documentation on (repo) (if applicable)
Changelog has been updated and is aligned with our changelog requirements

Relates to #2737

v-shenoy · 2022-05-30T06:31:13Z

Sorry for the delay here, but here's a sample PR with E2E tests in Go.

I have also added sample logs from a test run to see how they look. I feel that the logs are a lot cleaner as well compared to the ones from ava.

@tomkerkhove @JorTurFer @zroubalik

JorTurFer · 2022-05-30T20:32:32Z

Sorry for the delay here, but here's a sample PR with E2E tests in Go.

I have also added sample logs from a test run to see how they look. I feel that the logs are a lot cleaner as well compared to the ones from ava.

@tomkerkhove @JorTurFer @zroubalik

I'm a bit busy atm, but I'll review it ASAP, huge kudos for this improvement! ❤️ ❤️ ❤️ ❤️

v-shenoy · 2022-05-31T05:03:29Z

I'm a bit busy atm, but I'll review

That would be great. If the sample looks okay, I will start working on migrating all the other Azure scalers as a part of this PR as well. After that the community can start working on others, and help with the migration.

zroubalik

Looking good! Great job :)

We should find a way how to run these in parallel until the full migration is done.

e2e/helper.go

zroubalik · 2022-05-31T08:34:27Z

Could you please also update the https://github.com/kedacore/keda/blob/main/tests/README.md in this PR as well?

v-shenoy · 2022-05-31T08:37:15Z

Looking good! Great job :)

We should find a way how to run these in parallel until the full migration is done.

Could you explain what you mean by this? The run-all.sh file does run the tests in parallel, right?

Or do you mean running the go and ts files in parallel until the full migration is done?

zroubalik · 2022-05-31T08:39:58Z

Looking good! Great job :)
We should find a way how to run these in parallel until the full migration is done.

Could you explain what you mean by this? The run-all.sh file does run the tests in parallel, right?

Or do you mean running the go and ts files in parallel until the full migration is done?

yeah, but it's more a matter of modifying GH Workflows. @JorTurFer might have better idea. Let's solve this once this PR is ready.

v-shenoy · 2022-05-31T08:40:05Z

Could you please also update the https://github.com/kedacore/keda/blob/main/tests/README.md in this PR as well?

Currently, I kept these tests under a folder e2e in the root.
I am thinking of moving these to tests/e2e_go until the complete migration.

After the complete migration, we can remove the contents of tests and rename tests/e2e_go -> tests.

And yes, I will add a readme to tests/e2e_go/README.md explaining the testing.

zroubalik · 2022-05-31T08:40:54Z

Could you please also update the https://github.com/kedacore/keda/blob/main/tests/README.md in this PR as well?

Currently, I kept these tests under a folder e2e in the root. I am thinking of moving these to tests/e2e_go until the complete migration.

After the complete migration, we can remove the contents of tests and rename them tests/e2e_go -> tests.

And yes, I will add a readme to tests/e2e_go/README.md explaining the testing.

sounds like a good plan!

v-shenoy · 2022-05-31T08:42:17Z

Looking good! Great job :)
We should find a way how to run these in parallel until the full migration is done.

Could you explain what you mean by this? The run-all.sh file does run the tests in parallel, right?
Or do you mean running the go and ts files in parallel until the full migration is done?

yeah, but it's more a matter of modifying GH Workflows. @JorTurFer might have better idea. Let's solve this once this PR is ready.

The only concern I have is running both the workflows will increase time required. I think what might be a good approach is, letting the main build that gets run on a PR merge not be changed until complete migration is done.

And, for PRs have a temporary separate command such as /run-e2e-go in parallel to /run-e2e. Once the migration is done we can replace the /run-e2e workflow with the other one.

JorTurFer · 2022-05-31T18:23:09Z

The only concern I have is running both the workflows will increase time required. I think what might be a good approach is, letting the main build that gets run on a PR merge not be changed until complete migration is done.

And, for PRs have a temporary separate command such as /run-e2e-go in parallel to /run-e2e. Once the migration is done we can replace the /run-e2e workflow with the other one.

uff... Adding a second command could be a pain TBH. The command is a super custom workload that I did, and duplicate it could be horrible. Maybe we can just run both all the time due to we are going to move the tests from one to another, so once we have moved the test to Golang, it shouldn't exist for a long time in typescript.
Other option could be just combining both e2e test files in same folder. Thay have different extensions so we can know which one is each one based on the extension, and having all together can simplify the moving process.
When we add a new e2e test in go, we can just delete the ts file.
WDYT?

JorTurFer · 2022-05-31T19:04:31Z

One more thing, the command make test is also executing the new e2e tests. Maybe we have to add build tags

zroubalik · 2022-05-31T19:05:39Z

I would go the simplest way - execute both test suites and always delete overlapping tests.

JorTurFer · 2022-05-31T19:24:11Z

I would go the simplest way - execute both test suites and always delete overlapping tests.

Agree. The best part of this is that make e2e-test will continue being the way to trigger them, so main-build and nightly build still work without any other change

v-shenoy · 2022-06-03T12:27:10Z

I am keeping the go tests in scalers_go and the ts tests in scalers. I have modified the run-all.sh file to run both go and ts files until the migration is complete.

Replaced the setup.test.ts and cleanup.test.ts with setup_test.go and cleanup_test.go. And I everytime some adds a test for a scaler which already has a ts test, they should delete the ts file and only keep the go file.

I have also added a section in the README with some instructions and a small example. It isn't the best, but I will try to improve it in the future.

@JorTurFer @zroubalik

v-shenoy · 2022-06-03T12:28:24Z

I will be adding these tests as a part of this PR (hopefully by Monday/Tuesday).

App Insights
Blob
Data Explorer
Key Vault Queue
Log Analytics
Pipelines
Queue Idle Replicas
Queue Pause At N
Queue Pause
Queue Restore Original Replicas
Queue Trigger Auth
Queue
Service Bus Queue Workload Identity
Service Bus Queue
Service Bus Topic

And hopefully once this PR is merged, the community steps up in helping with the migration and takes up the other scalers.

v-shenoy · 2022-06-03T13:05:06Z

The tests folder will have to be skipped during pr and main workflows, else the go files for the e2e tests will also be run.

zroubalik · 2022-06-06T06:31:57Z

I will be adding these tests as a part of this PR (hopefully by Monday/Tuesday).
* App Insights

* Blob

* Data Explorer

* Key Vault Queue

* Log Analytics

* Pipelines

* Queue Idle Replicas

* Queue Pause At N

* Queue Pause

* Queue Restore Original Replicas

* Queue Trigger Auth

* Queue

* Service Bus Queue Workload Identity

* Service Bus Queue

* Service Bus Topic
And hopefully once this PR is merged, the community steps up in helping with the migration and takes up the other scalers.

@v-shenoy do you think that you can add only a subset of these tests in this PR? There are some new scalers that could already use the golang version. You can add the rest of the Azure related tests in a subsequent PR. WDYT?

zroubalik · 2022-06-06T06:33:12Z

The tests folder will have to be skipped during pr and main workflows, else the go files for the e2e tests will also be run.

Please do that in this PR (also include nightly).

v-shenoy · 2022-06-06T06:45:28Z

I will be adding these tests as a part of this PR (hopefully by Monday/Tuesday).
* App Insights

* Blob

* Data Explorer

* Key Vault Queue

* Log Analytics

* Pipelines

* Queue Idle Replicas

* Queue Pause At N

* Queue Pause

* Queue Restore Original Replicas

* Queue Trigger Auth

* Queue

* Service Bus Queue Workload Identity

* Service Bus Queue

* Service Bus Topic
And hopefully once this PR is merged, the community steps up in helping with the migration and takes up the other scalers.
@v-shenoy do you think that you can add only a subset of these tests in this PR? There are some new scalers that could already use the golang version. You can add the rest of the Azure related tests in a subsequent PR. WDYT?

Alright, I can add the Service Bus (Queue + Topic + Workload) and Key Vault tests. And then add the rest later. Does that seem good?

zroubalik · 2022-06-06T07:07:09Z

@v-shenoy yeah, thanks 🙏

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

v-shenoy · 2022-06-10T11:37:12Z

/run-e2e
Update: You can check the progres here

v-shenoy · 2022-06-10T11:38:41Z

/run-e2e Update: You can check the progres here

I only just remembered I could also run tests, haha. Well, it works, and is going to be really helpful :D

@JorTurFer

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

v-shenoy · 2022-06-10T13:49:05Z

/run-e2e
Update: You can check the progres here

JorTurFer · 2022-06-10T14:06:35Z

Another thing, I was worried about how to deal with panics, because if a test panics the cleanup code may not run. And this might leave the test resources in an unexpected state. But, this is no different than a test in ava with incorrect cleanup code.

I think that this problem is solved with this deletion "just in case" at the end of every e2e process 😄

JorTurFer · 2022-06-10T14:47:27Z

I only just remembered I could also run tests, haha. Well, it works, and is going to be really helpful :D

😄

JorTurFer · 2022-06-10T14:50:24Z

I have to recognize that I love the improvement in the loging, I mean, something as simple as like Current - 0, Target - 1 will help a lot IMO. Super nice improvement!

JorTurFer · 2022-06-10T15:15:57Z

I'm thinking on what will happen in case of panic, and I'm not sure if we will face with stuck namespaces 🤔
I mean, we will try to delete all e2e nasmapces. but if there is any ScaledObject or ScaledJob at that moment, the namespace will be stuck because the finalizers will block the deletion.
Does Kustomize follow any criteria like 'delete CRDs before the others'?

v-shenoy · 2022-06-10T15:25:23Z

/run-e2e Update: You can check the progres here

The tests are passing, whew!

v-shenoy · 2022-06-10T15:33:01Z

I'm thinking on what will happen in case of panic, and I'm not sure if we will face with stuck namespaces 🤔 I mean, we will try to delete all e2e nasmapces. but if there is any ScaledObject or ScaledJob at that moment, the namespace will be stuck because the finalizers will block the deletion. Does Kustomize follow any criteria like 'delete CRDs before the others'?

I am not sure if kustomize does this. But maybe you can add delete all scaled objects / jobs in namespaces with label type=e2e (or rather all namespaces just to be sure) in the make undeploy command before the kustomize operation.

JorTurFer · 2022-06-10T15:38:30Z

I was talking about doing something like this kubectl delete $(kubectl get scaledobjects,scaledjobs -oname) but including all the namespaces (I haven't tested the code below, I wrote in on-the-fly and maybe we need to adapt it):

while read -r namespace
do
    kubectl delete $(kubectl get scaledobjects,scaledjobs -oname -n "$namespace") -n "$namespace"
done < (kubectl get namespaces)

Doing this as part of the make undeploy before kustomize command should be enough to ensure that we delete all the KEDA resources with finalizers.
WDYT?

v-shenoy · 2022-06-10T15:39:38Z

I was talking about doing something like this kubectl delete $(kubectl get scaledobjects,scaledjobs -oname) but including all the namespaces (I haven't tested the code below, I wrote in on-the-fly and maybe we need to adapt it):
while read -r namespace
do
    kubectl delete $(kubectl get scaledobjects,scaledjobs -oname -n "$namespace") -n "$namespace"
done < (kubectl get namespaces)
Doing this as part of the make undeploy before kustomize command should be enough to ensure that we delete all the KEDA resources with finalizers. WDYT?

Yup, I was thinking something exactly like this.

zroubalik · 2022-06-10T15:41:06Z

Don't forget to include TriggerAuth... :) And then finally namespaces.

JorTurFer · 2022-06-10T15:43:28Z

Don't forget to include TriggerAuth... :)

TriggerAuth doesn't have finalizers, so I think that we can just delete them deleting the CRD, the problems are the resources with finalizers :/
I mean, delete de CRD will try to delete every resource, but the SO/SJ finalizers block the deletion if KEDA was killed before

v-shenoy · 2022-06-10T16:02:33Z

Don't forget to include TriggerAuth... :) And then finally namespaces.

The namespaces get deleted with make e2e-test clean already because of the e2e label, as long as they were created with the CreateNamespace helper method.

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

v-shenoy · 2022-06-11T11:33:25Z

/run-e2e
Update: You can check the progress here

v-shenoy · 2022-06-11T13:14:05Z

/run-e2e Update: You can check the progress here

@JorTurFer @zroubalik I have added a make target e2e-test-clean-crds that deletes any scaled objects, jobs that are on the cluster using a shell script. I have checked that the script works with resources across multiple namespaces. And added this new target as a dependency for undeploy. You can see that it's running here.

The tests also seem to be running fine.

Any more changes expected in this PR?

JorTurFer

LGTM!
Thanks for this incredible improvement ❤️

zroubalik

LGTM, awesome stuff! 🚀

v-shenoy requested a review from a team as a code owner May 30, 2022 06:28

v-shenoy marked this pull request as draft May 30, 2022 06:30

zroubalik reviewed May 31, 2022

View reviewed changes

e2e/helper.go Outdated Show resolved Hide resolved

e2e/helper.go Outdated Show resolved Hide resolved

zroubalik mentioned this pull request May 31, 2022

Add Apache Pulsar Scaler #3021

Merged

5 tasks

zroubalik mentioned this pull request Jun 1, 2022

Add support to customize HPA name #3068

Merged

5 tasks

v-shenoy force-pushed the e2e-go branch from 0f091ff to f3afb6f Compare June 3, 2022 12:27

zroubalik mentioned this pull request Jun 6, 2022

Add AWS DynamoDB Streams Scaler #3121

Merged

5 tasks

v-shenoy added 3 commits June 10, 2022 15:15

Replace uses of go-playground/assert/v2 with testify/assert for parity.

821c421

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

Update test runner scripts.

f5204c3

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

Sort environment variables in .env file.

3da8d15

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

Fix unbound variable error.

0b6dcd9

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

v-shenoy added 2 commits June 10, 2022 18:15

Remove report.xml

15a3e8c

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

Exclude setup and cleanup files from runner scripts.

c3dbb82

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

Cleanup CRDs.

c9641e7

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

JorTurFer approved these changes Jun 12, 2022

View reviewed changes

v-shenoy requested a review from zroubalik June 12, 2022 14:56

zroubalik approved these changes Jun 13, 2022

View reviewed changes

zroubalik merged commit 5da8dc1 into kedacore:main Jun 13, 2022

v-shenoy deleted the e2e-go branch June 13, 2022 07:14

Start migrating e2e tests to Go. #3079

Start migrating e2e tests to Go. #3079

Conversation

v-shenoy commented May 30, 2022 • edited Loading

Checklist

v-shenoy commented May 30, 2022

JorTurFer commented May 30, 2022

v-shenoy commented May 31, 2022

zroubalik left a comment

Choose a reason for hiding this comment

zroubalik commented May 31, 2022

v-shenoy commented May 31, 2022

zroubalik commented May 31, 2022

v-shenoy commented May 31, 2022 • edited Loading

zroubalik commented May 31, 2022 • edited Loading

v-shenoy commented May 31, 2022

JorTurFer commented May 31, 2022

JorTurFer commented May 31, 2022

zroubalik commented May 31, 2022

JorTurFer commented May 31, 2022

v-shenoy commented Jun 3, 2022

v-shenoy commented Jun 3, 2022 • edited Loading

v-shenoy commented Jun 3, 2022 • edited Loading

zroubalik commented Jun 6, 2022

zroubalik commented Jun 6, 2022

v-shenoy commented Jun 6, 2022 • edited Loading

zroubalik commented Jun 6, 2022

v-shenoy commented Jun 10, 2022 • edited by github-actions bot Loading

v-shenoy commented Jun 10, 2022 • edited Loading

v-shenoy commented Jun 10, 2022 • edited by github-actions bot Loading

JorTurFer commented Jun 10, 2022

JorTurFer commented Jun 10, 2022

JorTurFer commented Jun 10, 2022

JorTurFer commented Jun 10, 2022

v-shenoy commented Jun 10, 2022 • edited Loading

v-shenoy commented Jun 10, 2022 • edited Loading

JorTurFer commented Jun 10, 2022 • edited Loading

v-shenoy commented Jun 10, 2022

zroubalik commented Jun 10, 2022 • edited Loading

JorTurFer commented Jun 10, 2022 • edited Loading

v-shenoy commented Jun 10, 2022 • edited Loading

v-shenoy commented Jun 11, 2022 • edited by github-actions bot Loading

v-shenoy commented Jun 11, 2022 • edited Loading

JorTurFer left a comment

Choose a reason for hiding this comment

zroubalik left a comment • edited Loading

Choose a reason for hiding this comment

v-shenoy commented May 30, 2022 •

edited

Loading

v-shenoy commented May 31, 2022 •

edited

Loading

zroubalik commented May 31, 2022 •

edited

Loading

v-shenoy commented Jun 3, 2022 •

edited

Loading

v-shenoy commented Jun 3, 2022 •

edited

Loading

v-shenoy commented Jun 6, 2022 •

edited

Loading

v-shenoy commented Jun 10, 2022 •

edited by github-actions bot

Loading

v-shenoy commented Jun 10, 2022 •

edited

Loading

v-shenoy commented Jun 10, 2022 •

edited by github-actions bot

Loading

v-shenoy commented Jun 10, 2022 •

edited

Loading

v-shenoy commented Jun 10, 2022 •

edited

Loading

JorTurFer commented Jun 10, 2022 •

edited

Loading

zroubalik commented Jun 10, 2022 •

edited

Loading

JorTurFer commented Jun 10, 2022 •

edited

Loading

v-shenoy commented Jun 10, 2022 •

edited

Loading

v-shenoy commented Jun 11, 2022 •

edited by github-actions bot

Loading

v-shenoy commented Jun 11, 2022 •

edited

Loading

zroubalik left a comment •

edited

Loading