Skip to content
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

Handle Windows panics in acceptance test exit status checks #1632

Closed
teor2345 opened this issue Jan 24, 2021 · 3 comments · Fixed by #1535
Closed

Handle Windows panics in acceptance test exit status checks #1632

teor2345 opened this issue Jan 24, 2021 · 3 comments · Fixed by #1535
Labels
A-rust Area: Updates to Rust code C-bug Category: This is a bug

Comments

@teor2345
Copy link
Contributor

teor2345 commented Jan 24, 2021

Is your feature request related to a problem? Please describe.

In the zebrad acceptance tests, Windows tasks that panic seem to return the same exit code as processes that are killed by the test runner.

Edit: We discovered this issue happened because we were calling kill on a task that had already panicked.

Describe the solution you'd like

Stop calling kill on that task.

Describe alternatives you've considered

We could do nothing, and our tests would be unreliable on Windows (or our test code would be hard to understand)

Additional context

Discovered when fixing #1535

@teor2345 teor2345 added C-bug Category: This is a bug A-rust Area: Updates to Rust code S-blocked Status: Blocked on other tasks S-needs-triage Status: A bug report needs triage labels Jan 24, 2021
@oxarbitrage
Copy link
Contributor

I think i found the cause of this bug and fixed it at df0e38d

Our acceptance test function check_config_conflict() was killing both nodes but node2 is already killed by the port conflict panic. Linux seem to dont care about this, it does not fail and the status code remains as not killed. Windows in the other hand mark the program as killed. So, i removed the killing of the second node and also the windows conditional.

@teor2345
Copy link
Contributor Author

Great, thanks for fixing this bug, it would have confused us if we copied that code around.

@teor2345 teor2345 removed the S-blocked Status: Blocked on other tasks label Jan 27, 2021
@teor2345
Copy link
Contributor Author

I've edited the task description and removed the blocker.

@teor2345 teor2345 added P-High and removed S-needs-triage Status: A bug report needs triage labels Jan 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust Area: Updates to Rust code C-bug Category: This is a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants