pipeline
pipeline copied to clipboard
tektoncd
Skipped Integration Tests
While working on PR https://github.com/tektoncd/pipeline/pull/6072, I noticed that 5 alpha integration tests were skipped.
Of the five, two were because they require embedded-status: full and enable-provenance-in-status: true. However, the kind-e2e tests set the feature flags to specific values for all integration tests. Therefore, these tests are skipped as a result and could potentially lead us to merge broken code.
I think this is surfacing now because we have many independent feature flags like results-from, embedded-status, enable-provenance-in-status etc. and some alpha features require one set of values while other alpha features could require a different value.
I think we require some restructuring of our integration tests.
WDYT?
/kind bug
Good point! I also encounter this before but didn't think this as a bug.
I think one option is: those tests need feature flags not set by the script could set them in the test itself like: https://github.com/tektoncd/pipeline/blob/ea358a6e18fdbbbc540053a04dbbfc56f9ceebb3/test/trusted_resources_test.go#L435-L437 And reset the config after the test is done.
This is based on the tests are not running in parallel
Yes, I agree. That's what needs to happen. I needed to do that for larger results.
However, after the test is complete, we also need to unset those feature flags and set them back to what they were. Otherwise, tests that follow will get the feature flags that you set in the tests and it can break them. This is because the feature flags are applied to the namespace tekton-pipelines instead of arendelle....
But as you can see, it is easy to miss and prone to errors. I was wondering if we should rethink the strategy where instead of having requireAllGates or requireAnyGate which can lead to skipping of tests, we explicitly set the feature flag that the test needs and then set it back to default when that test is complete.
Again, just a thought. It might cause more issues than what I foresee at the surface level.
Yes, I agree. That's what needs to happen. I needed to do that for larger results.
However, after the test is complete, we also need to unset those feature flags and set them back to what they were. Otherwise, tests that follow will get the feature flags that you set in the tests and it can break them. This is because the feature flags are applied to the namespace
tekton-pipelinesinstead ofarendelle....But as you can see, it is easy to miss and prone to errors. I was wondering if we should rethink the strategy where instead of having
requireAllGates or requireAnyGatewhich can lead to skipping of tests, we explicitly set the feature flag that the test needs and then set it back to default when that test is complete.Again, just a thought. It might cause more issues than what I foresee at the surface level.
I think we are talking about the same solution. 😅 Set the flag and reset when that test is done.
One other thing I found is that in order to fix the broken tests, I had to remove t.Parallel() so that it does not run in parallel with the other tests. This is because when my test updated the feature flags, it affected other tests running in parallel that would error when running along side mine.
One other thing I found is that in order to fix the broken tests, I had to remove
t.Parallel()so that it does not run in parallel with the other tests. This is because when my test updated the feature flags, it affected other tests running in parallel that would error when running along side mine.
yeah, I also mentioned that in my previous comment. I think this is important.
This is based on the tests are not running in parallel
I wanted to add that while this is doable for e2e tests, we cannot do this for example tests since all example tests will have the same feature flags for all.
I guess, my main point is if we think it's ok for this process to be as is or if we think it should be improved.
At the very least, we should document this process in the developer documentation.
Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.
/lifecycle stale
Send feedback to tektoncd/plumbing.
Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.
/lifecycle rotten
Send feedback to tektoncd/plumbing.
Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen with a justification.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.
/close
Send feedback to tektoncd/plumbing.
@tekton-robot: Closing this issue.
In response to this:
Rotten issues close after 30d of inactivity. Reopen the issue with
/reopenwith a justification. Mark the issue as fresh with/remove-lifecycle rottenwith a justification. If this issue should be exempted, mark the issue as frozen with/lifecycle frozenwith a justification./close
Send feedback to tektoncd/plumbing.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
