Add tests for create source git #1696

allenporter · 2021-08-07T04:54:10Z

Add unit tests for create source git.

The create source tests are more interesting than the existing unit tests as they
create objects then wait for the flux source reconciler to complete. The tests
simulate this with a background goroutine that waits for an object to be
created then uses a test specific function to update it. The reconciler was
implemented assuming that it might be possible to make it more generic
in the future if needed by additional tests.

The tests set a timeout so that if there is a failure they timeout somewhat
quickly rather than hanging for a longer period of time.

Issue #1603

allenporter · 2021-08-07T18:51:38Z

@chanwit as an FYI

chanwit · 2021-08-08T05:00:41Z

Thank you, Alen.

…

On Sun, Aug 8, 2021, 1:51 AM Allen Porter ***@***.***> wrote: @chanwit <https://github.com/chanwit> as an FYI — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1696 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAACTKWAZB2AEGLIUJ6RHDTT3V6ELANCNFSM5BXAXKOA> .

hiddeco

LGTM, and seems to be indeed the fix for the other test-related PR :-)

allenporter · 2021-08-17T05:04:33Z

@hiddeco i'm having trouble merging this with the e2e tests that were just added. Do you have a workflow for running the e2e tests with extra logging enabled? My iteration cycle is pretty high having to wait for github actions to run -- or perhaps you can lend a pair of eyes and spot if i added a bug.

hiddeco · 2021-08-17T09:49:19Z

@chanwit @stefanprodan anything standing out from the PR that got merged into main that could cause conflicts with this one? I can't find an obvious reason on a first scan.

stefanprodan · 2021-08-17T10:31:51Z

We can’t use the install test, I’m dropping it, PR will follow.

stefanprodan · 2021-08-17T11:02:52Z

Please see #1721

stefanprodan · 2021-08-17T12:40:55Z

@allenporter @hiddeco @chanwit I'm for dropping the mock API and use envtest.

chanwit · 2021-08-17T12:52:43Z

No objection @stefanprodan

stefanprodan · 2021-08-17T12:56:11Z

In the latest controller-runtime release, the testenv startup time has improve a lot, I think we need to port the flux trace from mock the API to testenv (without kind) and compare the timing.

allenporter · 2021-08-17T15:07:36Z

In the latest controller-runtime release, the testenv startup time has improve a lot, I think we need to port the flux trace from mock the API to testenv (without kind) and compare the timing.

1: On Testenv

Great, as I mentioned up on the other PR, having fewer test environments would be ideal if its speedy. Now that the e2e and unit tests share the same harness, but not the same setup, it seems easy to accidentally break the e2e tests while iterating on the unit tests, so removing the differences would be nice.

@chanwit hav you exercised TestEnvClusterMode to run tests locally yet? I could use a few pointers to get started and understanding how the Makefile is supposed to work:

$ make setup-envtest SHELL='sh -x'
+ which setup-envtest
make: Nothing to be done for 'setup-envtest'.

2: On running e2e tests, back to my question above: Do you have a workflow for running the e2e tests locally? I'm having difficulty telling if the e2e tests are still flaky or i'm breaking it. I'm assuming they are still flaky given the error messages are related to deadline exceeded. I don't seem to be able to Retry jobs (maybe a permissions thing).

3: Stepping back: The approach in this PR is to be able to support more complex test types to be able to exercise edge cases such as waiting for the results of a controller to run. I keep seeing more PRs coming in adding tests with the old harness in parallel with this so I'd like to make sure this is useful before doing more catchup. Is this a useful direction or should I abandon this?

cmd/flux/install_test.go

hiddeco · 2021-08-17T15:21:48Z

Is this a useful direction or should I abandon this?

I think the new direction (yours) is more useful and we should probably look into transforming the others.

allenporter · 2021-08-17T15:27:46Z

Is this a useful direction or should I abandon this?

I think the new direction (yours) is more useful and we should probably look into transforming the others.

OK if we get the unit tests using testenv then maybe there will be less of a distinction between the two scenarios and we can leave a few e2e tests.

stefanprodan · 2021-08-17T16:10:46Z

Looks like the client is not wired correctly now, no types are registered.

allenporter · 2021-08-17T20:36:45Z

Looks like the client is not wired correctly now, no types are registered.

Correct -- the changes to the test harness to support the e2e end test have broken the fake harness wiring via rootCtx.kubeManager.

More detail:

main_e2e_test invokes the test setup and rootCtx.kubeManager We can call this testEnv #1
The tests themselves run again and run cmdTestCase.runTestCmd which also sets up the test environment and overrides rootCtx.kubeManager again. We can call this testEnv #2
The PR to update the e2e harness introduced a test that calls create source git which is the same thing that is being tested in this PR coincidentally
This PR updates create source git to use the rootCtx.kubeManager to create the client library for the test since its tested with the fake. That has the side effect of making this command use testEnv #2
Most of the code has not yet been updated to use rootCtx.kubeManager so it works as is with the e2e test by setting the flags and always using testEnv #1

I'll start unwinding this...

Replace the 4 arguments to cmdTestCase with a function that can let tests run arbitrary logic if it is more complex than what is provided by the test harness. Move the existing logic into functions that the test can use for common checks on golden files and golden values. These changes were pulled out of PR fluxcd#1696 to make a smaller review. Signed-off-by: Allen Porter <allen@thebends.org>

Replace the 4 arguments to cmdTestCase with a function that can let tests run arbitrary logic if it is more complex than what is provided by the test harness. Move the existing logic into functions that the test can use for common assertions on golden files and golden values. These changes were pulled out of PR fluxcd#1696 to make a smaller review. Signed-off-by: Allen Porter <allen@thebends.org>

The create source tests are more interesting than the existing tests as they create objects then wit for the flux source reconciler to complete. The tests simulate this with a background goroutine that waits for an object to be created then uses a test specific function to update it. The tests set a timeout so that if there is a failure they timeout somewhat quickly rather than hanging for a longer period of time. Signed-off-by: Allen Porter <allen@thebends.org>

hiddeco

LGTM, thank you @allenporter 💯

Replace the 4 arguments to cmdTestCase with a function that can let tests run arbitrary logic if it is more complex than what is provided by the test harness. Move the existing logic into functions that the test can use for common assertions on golden files and golden values. These changes were pulled out of PR fluxcd#1696 to make a smaller review. Signed-off-by: Allen Porter <allen@thebends.org>

allenporter force-pushed the flux-cmd-create branch from e2ca618 to f61b91e Compare August 7, 2021 04:54

allenporter changed the title ~~Add tests for create source git~~ Add tests for create source git Aug 7, 2021

allenporter force-pushed the flux-cmd-create branch from f61b91e to 82c06d0 Compare August 7, 2021 05:08

allenporter changed the title ~~Add tests for create source git~~ Add tests for create source git and make test harness more flexible Aug 12, 2021

allenporter mentioned this pull request Aug 12, 2021

Adding tests to flux create secret commands #1699

Closed

hiddeco approved these changes Aug 12, 2021

View reviewed changes

allenporter mentioned this pull request Aug 17, 2021

Implement testEnv for e2e tests #1697

Merged

allenporter force-pushed the flux-cmd-create branch 5 times, most recently from 24850ac to 9ede6e0 Compare August 17, 2021 04:49

allenporter force-pushed the flux-cmd-create branch from 9ede6e0 to 29d6646 Compare August 17, 2021 14:25

stefanprodan reviewed Aug 17, 2021

View reviewed changes

cmd/flux/install_test.go Outdated Show resolved Hide resolved

allenporter force-pushed the flux-cmd-create branch from 29d6646 to fcf2e64 Compare August 17, 2021 15:19

allenporter force-pushed the flux-cmd-create branch from fcf2e64 to 8d9cba4 Compare August 17, 2021 15:38

allenporter marked this pull request as draft August 17, 2021 20:07

allenporter force-pushed the flux-cmd-create branch from 0b432dd to 4ee18da Compare August 18, 2021 04:14

allenporter mentioned this pull request Aug 23, 2021

Make test harness more flexible with functions #1739

Merged

allenporter force-pushed the flux-cmd-create branch from 4ee18da to 25d27a1 Compare August 24, 2021 19:36

allenporter changed the title ~~Add tests for create source git and make test harness more flexible~~ Add tests for create source git Aug 24, 2021

allenporter force-pushed the flux-cmd-create branch from 25d27a1 to 6e5d39c Compare August 24, 2021 19:51

allenporter marked this pull request as ready for review August 24, 2021 19:53

allenporter force-pushed the flux-cmd-create branch 2 times, most recently from 8200ac3 to 4d0a1e8 Compare August 25, 2021 20:16

allenporter force-pushed the flux-cmd-create branch from 4d0a1e8 to 8cbd4e8 Compare August 30, 2021 14:44

allenporter requested review from hiddeco and stefanprodan August 30, 2021 14:44

hiddeco approved these changes Aug 30, 2021

View reviewed changes

hiddeco merged commit 06fa8f7 into fluxcd:main Aug 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for create source git #1696

Add tests for create source git #1696

allenporter commented Aug 7, 2021 •

edited

allenporter commented Aug 7, 2021

chanwit commented Aug 8, 2021 via email

hiddeco left a comment

allenporter commented Aug 17, 2021

hiddeco commented Aug 17, 2021

stefanprodan commented Aug 17, 2021

stefanprodan commented Aug 17, 2021

stefanprodan commented Aug 17, 2021

chanwit commented Aug 17, 2021

stefanprodan commented Aug 17, 2021

allenporter commented Aug 17, 2021 •

edited

hiddeco commented Aug 17, 2021

allenporter commented Aug 17, 2021

stefanprodan commented Aug 17, 2021

allenporter commented Aug 17, 2021 •

edited

hiddeco left a comment

Add tests for create source git #1696

Add tests for create source git #1696

Conversation

allenporter commented Aug 7, 2021 • edited

allenporter commented Aug 7, 2021

chanwit commented Aug 8, 2021 via email

hiddeco left a comment

Choose a reason for hiding this comment

allenporter commented Aug 17, 2021

hiddeco commented Aug 17, 2021

stefanprodan commented Aug 17, 2021

stefanprodan commented Aug 17, 2021

stefanprodan commented Aug 17, 2021

chanwit commented Aug 17, 2021

stefanprodan commented Aug 17, 2021

allenporter commented Aug 17, 2021 • edited

hiddeco commented Aug 17, 2021

allenporter commented Aug 17, 2021

stefanprodan commented Aug 17, 2021

allenporter commented Aug 17, 2021 • edited

hiddeco left a comment

Choose a reason for hiding this comment

allenporter commented Aug 7, 2021 •

edited

allenporter commented Aug 17, 2021 •

edited

allenporter commented Aug 17, 2021 •

edited