Re: [PR] Test fixes [camel-k]
valdar merged PR #5415: URL: https://github.com/apache/camel-k/pull/5415 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Test fixes [camel-k]
valdar commented on code in PR #5415:
URL: https://github.com/apache/camel-k/pull/5415#discussion_r1594000383
##
pkg/platform/defaults.go:
##
@@ -325,7 +325,7 @@ func setPlatformDefaults(p *v1.IntegrationPlatform, verbose
bool) error {
// Build timeout
if p.Status.Build.GetTimeout().Duration == 0 {
p.Status.Build.Timeout = {
- Duration: 5 * time.Minute,
+ Duration: 10 * time.Minute,
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Test fixes [camel-k]
squakez commented on code in PR #5415:
URL: https://github.com/apache/camel-k/pull/5415#discussion_r1579704776
##
pkg/platform/defaults.go:
##
@@ -325,7 +325,7 @@ func setPlatformDefaults(p *v1.IntegrationPlatform, verbose
bool) error {
// Build timeout
if p.Status.Build.GetTimeout().Duration == 0 {
p.Status.Build.Timeout = {
- Duration: 5 * time.Minute,
+ Duration: 10 * time.Minute,
Review Comment:
Good one. This is changing the default existing behavior, not sure if it's
something we should introduce so lightly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Test fixes [camel-k]
christophd commented on PR #5415: URL: https://github.com/apache/camel-k/pull/5415#issuecomment-2077130633 I think the native tests tend to not work in GH actions, do they? @valdar Maybe you can run the native tests on your machine and give feedback here when they succeeded? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Test fixes [camel-k]
christophd commented on code in PR #5415:
URL: https://github.com/apache/camel-k/pull/5415#discussion_r1579175621
##
pkg/platform/defaults.go:
##
@@ -325,7 +325,7 @@ func setPlatformDefaults(p *v1.IntegrationPlatform, verbose
bool) error {
// Build timeout
if p.Status.Build.GetTimeout().Duration == 0 {
p.Status.Build.Timeout = {
- Duration: 5 * time.Minute,
+ Duration: 10 * time.Minute,
Review Comment:
Maybe this is something we could just increase in the E2E tests by setting
this duration on the IntegrationPlatform instances created by the E2E tests?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Test fixes [camel-k]
christophd commented on code in PR #5415: URL: https://github.com/apache/camel-k/pull/5415#discussion_r1577904550 ## e2e/support/test_support.go: ## @@ -128,9 +128,9 @@ const ExpectedOSClusterRoles = 1 var TestDefaultNamespace = "default" -var TestTimeoutShort = 1 * time.Minute -var TestTimeoutMedium = 5 * time.Minute -var TestTimeoutLong = 15 * time.Minute +var TestTimeoutShort = 5 * time.Minute Review Comment: I see both of you having a point here. IMO if the increased timeout really fixes the flaky tests I tend to accept the fact that we need to wait some more time when a test really is broken by the changes in a PR. The feedback loop for a successful test is over 60 minutes anyways and having a flaky test can be really exhausting. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Test fixes [camel-k]
squakez commented on code in PR #5415: URL: https://github.com/apache/camel-k/pull/5415#discussion_r1577831238 ## e2e/support/test_support.go: ## @@ -128,9 +128,9 @@ const ExpectedOSClusterRoles = 1 var TestDefaultNamespace = "default" -var TestTimeoutShort = 1 * time.Minute -var TestTimeoutMedium = 5 * time.Minute -var TestTimeoutLong = 15 * time.Minute +var TestTimeoutShort = 5 * time.Minute Review Comment: That's exactly the problem. In case of an unstable change we may introduce, the time to complete a test would be at least duplicated, having to wait for a feedback too much time. Imho, the goal should be to have faster timeouts. If some test is not stable, then, I don't think the culprit is the timeout. And if it's the timeout, likely it's in the specific flaky test. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Test fixes [camel-k]
valdar commented on code in PR #5415: URL: https://github.com/apache/camel-k/pull/5415#discussion_r159400 ## e2e/support/test_support.go: ## @@ -128,9 +128,9 @@ const ExpectedOSClusterRoles = 1 var TestDefaultNamespace = "default" -var TestTimeoutShort = 1 * time.Minute -var TestTimeoutMedium = 5 * time.Minute -var TestTimeoutLong = 15 * time.Minute +var TestTimeoutShort = 5 * time.Minute Review Comment: According to my empirical tests these are the values that minimize test runs failing due to small perturbations in load or network speed. Honestly I don't see may cons in bumping them, the only one is that actually failing tests will take longer. But that is a condition that should happens rarely since most of the runs should be of passing tests. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Test fixes [camel-k]
valdar commented on code in PR #5415: URL: https://github.com/apache/camel-k/pull/5415#discussion_r159400 ## e2e/support/test_support.go: ## @@ -128,9 +128,9 @@ const ExpectedOSClusterRoles = 1 var TestDefaultNamespace = "default" -var TestTimeoutShort = 1 * time.Minute -var TestTimeoutMedium = 5 * time.Minute -var TestTimeoutLong = 15 * time.Minute +var TestTimeoutShort = 5 * time.Minute Review Comment: According to my empirical tests there are the one that minimize test runs failing due to small perturbations in load or network speed. Honestly I don't see may cons in bumping them, the only one is that actually failing tests will take longer. But that is a condition that should happens rarely since most of the runs should be of passing tests. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Test fixes [camel-k]
squakez commented on code in PR #5415: URL: https://github.com/apache/camel-k/pull/5415#discussion_r1577727640 ## e2e/support/test_support.go: ## @@ -128,9 +128,9 @@ const ExpectedOSClusterRoles = 1 var TestDefaultNamespace = "default" -var TestTimeoutShort = 1 * time.Minute -var TestTimeoutMedium = 5 * time.Minute -var TestTimeoutLong = 15 * time.Minute +var TestTimeoutShort = 5 * time.Minute Review Comment: I think these values are too high. If we need to change them specifically for certain tests, then, we should change the test and include the specific timeout instead. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
