add tests for controller with mock #344

priyakumarank · 2019-10-11T04:04:50Z

#99

What this PR does / why we need it:

Add tests for SQL Server Controller and SQL Database Controller
Update sqlclient_gosdk.go to be mockable
Update SQLFirewallRule Controller and SQLAction Controller to support changes in sqlclient_gosdk
Had to remove // +kubebuilder:subresource:status from sqlserver and sqldatabase type, as the tests were crashing(with and without mock) similar to what was happening in eventhubs until we removed it. this needs more investigation, and can possibly be done as a separate spike.
Sql Server and SQL Database Controller tests with run with mock always. Running it with Azure results in entire suite timing out. And the Azure end to end tests cover this scenario anyways. We should consider running all the controller tests using mock only. i.e. change the flag in test-existing.
Disable parallel tests in controller as it seems to lead to intermittent test failures

Special notes for your reviewer:

If applicable:

this PR contains documentation
this PR contains tests

…om/priyakumarank/azure-service-operator into feat/99-SQLControllerTestsWithMock

frodopwns

Status should remain a sub-resource. Any reason why you chose to remove it?

api/v1alpha1/azuresqldatabase_types.go

api/v1alpha1/azuresqlserver_types.go

frodopwns · 2019-10-15T15:40:54Z

Just saw your explanation for removing the sub-resource. I understand why you might want to do that but I'd rather we didn't. The tests failing before were reasonable and there was a fix but we just didn't implement it.

This is pretty important for the functionality the operators. We will probably update eventhub with it again eventually.

priyakumarank · 2019-10-20T21:27:03Z

Just saw your explanation for removing the sub-resource. I understand why you might want to do that but I'd rather we didn't. The tests failing before were reasonable and there was a fix but we just didn't implement it.
This is pretty important for the functionality the operators. We will probably update eventhub with it again eventually.

Fair enough. Investigating why the status subresource breaks the tests was going to be my next to do. I wanted to get this in as the first cut. I understand your concerns, so will split this PR into two, where the first one would get the mock client and the changes to the client to support mock in, so that it enables other team members to work on sql firewall and other sql tests. And the second PR would have the tests with the status subresource fix.

frodopwns

couple comments, I will test this tomorrow

pkg/resourcemanager/mock/sqlclient/sqlclient_gosdk.go

pkg/resourcemanager/sqlclient/sqlclient_manager.go

frodopwns

Looks good for the most part. You should put the sdkClient in the MockSQLManager instead of passing it as an argument though.

priyakumarank · 2019-10-24T03:05:40Z

Looks good for the most part. You should put the sdkClient in the MockSQLManager instead of passing it as an argument though.

I have updated the gosdkclient as discussed.

frodopwns · 2019-10-28T20:07:14Z

controllers/azuresqldatabase_controller_finalizer.go

+	}
+	r.Recorder.Event(instance, "Normal", "Updated", fmt.Sprintf("finalizer %s added", AzureSQLDatabaseFinalizerName))
+	return nil
+}


for future reference...we have some generic functions to replace these in the helper lib.

issues were addressed

frodopwns

looks good, thanks @priyakumarank !

If we panic when writing a file we end up with an empty (or worse) file. The empty file makes `header-check` fail and then you have to manually remove it to re-run any tests you’re doing. So: let’s remove it.

Priya Kumaran and others added 17 commits October 11, 2019 14:54

add tests for controller with mock

e8ecfb8

merge from master to branch

4d281cc

add package

3e6fe19

update api version in tests

485deb5

remove status update instance directly

6f593de

fix controllerutil crash in tests and move mock

7a78216

update to run mock and azure based on flag

c02963f

Merge branch 'master' into feat/99-SQLControllerTestsWithMock

71fc0ee

Merge branch 'master' into feat/99-SQLControllerTestsWithMock

6b1b30f

enable tests to run in parallel

81597ec

increase timeout

e2e213b

run sql controller with mock always

55bf533

merge from master to branch

fdf622b

rename test files

5756c4d

run sql with mock always

3372db8

update test

90aef7d

Merge branch 'master' into feat/99-SQLControllerTestsWithMock

5eb7841

priyakumarank marked this pull request as ready for review October 15, 2019 03:42

priyakumarank and others added 5 commits October 15, 2019 14:42

Merge branch 'master' into feat/99-SQLControllerTestsWithMock

f3ddfdf

enable parallel tests

e49f9eb

Merge branch 'feat/99-SQLControllerTestsWithMock' of https://github.c…

32af2b3

…om/priyakumarank/azure-service-operator into feat/99-SQLControllerTestsWithMock

run sql using mock

2f31e9f

disable tests running in parallel

4a429eb

priyakumarank requested review from frodopwns and jananivMS October 15, 2019 06:04

frodopwns suggested changes Oct 15, 2019

View reviewed changes

api/v1alpha1/azuresqldatabase_types.go Outdated Show resolved Hide resolved

api/v1alpha1/azuresqlserver_types.go Outdated Show resolved Hide resolved

merge from master to branch

706887d

remove tests and add status subresource back

64e1aca

priyakumarank requested a review from frodopwns October 21, 2019 21:26

Priya Kumaran added 3 commits October 22, 2019 08:57

merge from master and fix conflicts

fc325eb

add interface methods

06b4059

update status

d3dba00

frodopwns reviewed Oct 22, 2019

View reviewed changes

pkg/resourcemanager/mock/sqlclient/sqlclient_gosdk.go Outdated Show resolved Hide resolved

frodopwns reviewed Oct 22, 2019

View reviewed changes

pkg/resourcemanager/sqlclient/sqlclient_manager.go Outdated Show resolved Hide resolved

frodopwns previously requested changes Oct 22, 2019

View reviewed changes

Priya Kumaran added 5 commits October 23, 2019 16:46

refactor gosdkclient

8a4f788

refactor sqlclient gosdk

5dd33bd

use resourceclient

f777198

remove redundant file

f59f811

enable parallel tests

77c8ae5

priyakumarank requested a review from frodopwns October 24, 2019 03:05

Priya Kumaran and others added 6 commits October 25, 2019 13:21

add tests and fix issues

3088535

merge from master to branch

1ca646c

disable parallel tests

c441016

use requeue_after env variable

11ac815

Merge branch 'master' into feat/99-SQLControllerTestsWithMock

ca48085

make requeue time configurablle in eh ns

52a13d6

frodopwns reviewed Oct 28, 2019

View reviewed changes

frodopwns approved these changes Oct 28, 2019

View reviewed changes

priyakumarank merged commit a0dad08 into Azure:master Oct 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add tests for controller with mock #344

add tests for controller with mock #344

priyakumarank commented Oct 11, 2019 •

edited

Loading

frodopwns left a comment

frodopwns commented Oct 15, 2019 •

edited

Loading

priyakumarank commented Oct 20, 2019

frodopwns left a comment

frodopwns left a comment

priyakumarank commented Oct 24, 2019

frodopwns Oct 28, 2019

frodopwns left a comment

add tests for controller with mock #344

add tests for controller with mock #344

Conversation

priyakumarank commented Oct 11, 2019 • edited Loading

frodopwns left a comment

Choose a reason for hiding this comment

frodopwns commented Oct 15, 2019 • edited Loading

priyakumarank commented Oct 20, 2019

frodopwns left a comment

Choose a reason for hiding this comment

frodopwns left a comment

Choose a reason for hiding this comment

priyakumarank commented Oct 24, 2019

frodopwns Oct 28, 2019

Choose a reason for hiding this comment

frodopwns left a comment

Choose a reason for hiding this comment

priyakumarank commented Oct 11, 2019 •

edited

Loading

frodopwns commented Oct 15, 2019 •

edited

Loading