-
Notifications
You must be signed in to change notification settings - Fork 126
Add ignore_service_error flag to selectively ignore test service errored containers #1540
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ignore_service_error flag to selectively ignore test service errored containers #1540
Conversation
This will allow to selectively ignore or not an error in a test service container.
@@ -318,8 +318,7 @@ func (p *Project) Logs(opts CommandOptions) ([]byte, error) { | |||
func (p *Project) WaitForHealthy(opts CommandOptions) error { | |||
// Read container IDs | |||
args := p.baseArgs() | |||
args = append(args, "ps") | |||
args = append(args, "-q") | |||
args = append(args, "ps", "-a", "-q") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I encountered false positive failures for service containers that successfully finished but did so before reaching this point, so they would not be listed without -a
. Since exited but with 0
is considered healthy down the line, this is a requirement for these containers to show.
test integrations |
Created or updated PR in integrations repository to test this version. Check elastic/integrations#8421 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! This will help improving reliability of system tests. Overall LGTM.
we could argue that this should be default to false and then fix any failing tests due to this.
Agree, maybe we can do it? We could default to false in the PR, and run the tests on integrations (adding a test integrations
comment). If there are few tests failing, we can go on, and just add ignore_service_error: true
for these cases when updating the integrations repo.
test integrations |
Created or updated PR in integrations repository to test this version. Check elastic/integrations#8421 |
test integrations |
Created or updated PR in integrations repository to test this version. Check elastic/integrations#8421 |
💚 Build Succeeded
History
|
@jsoriano the failing ones in the test PR seem to be unrelated if I am not mistaken |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jsoriano the failing ones in the test PR seem to be unrelated if I am not mistaken
Good, let's change the default behaviour then to be on the safe side. Thanks for checking!
Adds a flag to selectively fail if a test service container exits with an error.
It currently defaults to true to preserve current behavior, but we could argue that this should be default to false and then fix any failing tests due to this.
Closes #1537