Separate out promotion tests #204
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
squaremo
force-pushed
the
separate-tests
branch
from
f6ec017
to
8ae22fc
Compare
squaremo
requested review from
yiannistri
and removed request for
luizbafilho
squaremo
force-pushed
the
separate-tests
branch
from
8ae22fc
to
5e11f46
Compare
This isolates the tests more, so they can be run selectively, and so they can use their own setup. Signed-off-by: Michael Bridgen <[email protected]>
These conditions ... - the pipeline reconciler is constructed in TestMain; - the test setup appends the mock strategy to the reconciler's strategies; - the mock strategy operates on values in its closure; - the first strategy answering `true` to Handle(...) will be used; ... all together mean that when you run the test case more than once, it will end up running the mock strategy on apps that aren't used any more. We decided earlier that replacing the whole set of strategies broke encapsulation too much; but, I think it may be the simplest way to make the test repeatable. These are alternatives I considered: - construct the reconciler anew for each test case (drags in the manager, then other things ...); - have a `Reset` method for the strategies (I don't like exporting methods just for testing); - refer to the app values indirectly, e.g., by having the mock strategy refer to a func which is re-assigned, rather than inlining it (would have to ensure the mock is installed once, but the func is reassigned -- awkward) Signed-off-by: Michael Bridgen <[email protected]>
The remote target tests check that a target app is marked as unready with an error of "not found" if it doesn't exist. But, running from scratch, first the target will be unready because the cache for that cluster has not synced yet, _then_ it will be unready because it's not found. When the tests are run locally, it goes through that first state fast enough that you probably wouldn't notice it; but in hosted test runs (i.e., in GitHub Actions), running a test env for the remote server is taxing enough that the flake becomes observable. Thus: use `Eventually` here to check that it reaches that state, rather than asserting it straight away. Signed-off-by: Michael Bridgen <[email protected]>
squaremo
force-pushed
the
separate-tests
branch
from
7d64eae
to
bd8b6d2
Compare
yiannistri
approved these changes
Nov 20, 2023
I just do This did need a tiny bit of preparation: I took the value of KUBEBUILDER_ASSETS from a run of |
|
Thanks, it was the KUBEBUILDER_ASSETS bit that prompted my question. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This isolates the tests more, so they can be run selectively, and so they can use their own setup.
As a side issue: we decided earlier that replacing the whole set of strategies broke encapsulation too much; but, I think it may be the simplest way to make the test repeatable. Otherwise, the test can't be run with
--count> 1. The dummy strategy is added each time, and refers to out-of-date app objects in its lexical closure. I have adjusted this.And as a diversion: the tests in controller_remote_test.go had an unreliable assumption, which I've fixed here because this is the PR where it was causing test failures most annoyingly.