e2e: test fixes and bug fixes from test runs #3716
Conversation
* Uses yq to insert the test bundle into existing channels as well as appending bundle into any new channels * Better replicates what the bundle will actually do to the channels in the bundle index
* Bump the operator-sdk version to 1.16 as being used to perform the generation * Updates the samples to the correct syntax
phantomjinx
force-pushed
the
main
branch
2 times, most recently
from
58e5d98
to
af841d8
Compare
| Eventually(PlatformPhase(ns), TestTimeoutLong).Should(Equal(v1.IntegrationPlatformPhaseReady)) | ||
| } | ||
|
|
||
| func TestKamelCLIRunGitHubExampleJava(t *testing.T) { |
There was a problem hiding this comment.
Why do we need to decompose the one TestKamelCLIRun() into smaller test funcs? Is it really necessary? Otherwise we'd keep one test func with multiple t.Run() style.
To me it looks a bit overkill to just fix one problematic test case "Run with http dependency".
There was a problem hiding this comment.
In the same way as TestRunConfig, the single test with run functions requires the clearing of the namespace at the end of each run - avoiding test contamination. The test executions seems to be less reilable in these circumstances since the delete might not clear everything before the next run is started. Since the WithNewNamespace has its own delete mechanism (deleting the whole namespace) and each test takes a fresh namespace the danger of test contamination is reduced and reliability improved.
There was a problem hiding this comment.
I am not convinced yet. From the point of view of test independence, it is obviously good to have separate test functions, but this creates a namespace each time, which then causes time efficiency problems due to the increased number of operator installations and the unavailability of downloaded jar caches and built kits in a new namespace. And we are already suffering from the long execution time of E2E tests. As a result of that trade-off, we've been trying to keep the tests together in E2E as much as possible (refer to the discussion #3298 if you are not aware of it). It is not consistent with the rest if we separate this test only here.
In the same way as TestRunConfig, the single test with run functions requires the clearing of the namespace at the end of each run - avoiding test contamination.
In the light of the above discussion, I don't see it particularly as a problem. And for this particular TestKamelCLIRun test, forcing to use a unique name for each test run would look like better improvement to avoid contamination for me.
If we had to separate the tests to solve the instability issue, then that would be a clear reason to separate them, but as far as I remember, this test hasn't been particularly unstable except this new one "Run with http dependency". Why can't we just fix the problematic test instead? Or has the TestKamelCLIRun test been flaky on OCP?
There was a problem hiding this comment.
Looking at the test times between 2 test runs, I can see the time differential for both TestKamelCLIRun and TestRunConfig is large. That being the case, converting the tests is not an ideal solution, despite improvements in reliability. So, I'll modify this back and relook at TestRunConfig (think the kamel delete.... might have to be improved to remove some test contamination).
* Extends the timeout for testing to 90m since if in debug mode, the extra logging increases the testing time and takes slightly longer than 60m * Adds in extra debug statements for logging when running in debug mode * Exposes the LOG_LEVEL parameter to e2e testsuite so tests can be switched to debug mode * Prints the call stack for kamel binary if running test in debug mode * Fixes the StructuredLogs test by better detection of invalid operator log entries by the test * Provide functions to heal a crashed catalogsource pod by waiting for its pull-secret to become available and then deleting the pod and allowing the source to reprovision a new one * Fixes uninstall test by recognising roles are not deleted when install was by the OLM * Fixes install test by checking the LOG_LEVEL env var rather than string to read the operator log which can be truncated * Fixes to tests by extending timeouts for integration pods coming up * Removes some cleanups from individual tests as these should be taken care of by the cleanup of the namespace * Better logging when building the bundle and bundle index images # Conflicts: # script/Makefile
* See issue apache#3667 for details * Can re-enable once solution has been concluded
* Separate test tasks into separate tests in different namespaces to avoid any contamination
* If test namespace exists then create a different one with extra suffix
* If operator is not uninstalled successfully then provide a warning rather than throw an error as we want to try to keep the tests going
* test_support.go * When installing for the tests, handle CAMEL_K_TEST_MAVEN_CLI_OPTIONS to inject maven-cli options specified by the tests * If e2e tests have a log-level of debug then set the maven-cli-options to "-X" to retrieve debugging from maven builds * Renames CAMEL_K_LOG_LEVEL with TEST prefix
* The default timeout for download timeouts was so quick as to make the test prone to failure. Increasing this timeout allows for more reliable tests.
* Rather than trying to delete resources at the end of each sub-test, it is simpler and more reliable to generate a new namespace for each and have it deleted * The sampleJar URL is changed for the http dependency tests to avoid the request have to do a redirect. This improves the reliability of its retrieval * Sets the http dependency tests to problematic since on OCP4, the repositories are not being detected by the maven build causing test failures. See apache#3708.
* The use of UpdateScale is unreliable in producing the well-known error "the object has been modified; please apply your changes to the latest version and try again". * Replacing UpdateScale with PatchScale avoids this error and allows the test to continue successfully
* While checking for the catalogue source pod, the function needs to assume that the pod may not yet exist.
|
@tadayosi If you are happy with the explanations and changes then could you, please, merge this? |
Release Note