Skip to content

Remove sporadicly failing test #99

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

Closed

Conversation

matthiasbeyer
Copy link
Member

This is obviously not how it is done. This patch will be merged to master so that we can work on master without issues, but a PR will be filed to revert this patch immediately... with a fix, if someone can come up with one.


This currently blocks the 0.6.0 release. I know that this is not how it is done, but I don't see a better way.

@matthiasbeyer
Copy link
Member Author

bors merge=petreeftime

bors bot added a commit that referenced this pull request Apr 15, 2023
99: Remove sporadicly failing test r=petreeftime a=matthiasbeyer

This is obviously not how it is done. This patch will be merged to master so that we can work on master without issues, but a PR will be filed to revert this patch immediately... with a fix, if someone can come up with one.

---

This currently blocks the 0.6.0 release. I know that this is not how it is done, but I don't see a better way.

Co-authored-by: Matthias Beyer <[email protected]>
@bors
Copy link
Contributor

bors bot commented Apr 15, 2023

Build failed:

@petreeftime
Copy link
Collaborator

petreeftime commented Apr 23, 2023

I think the issue might be due to some shared resource between tests. All tests execute on their own thread inside a single program and their order of execution is not guaranteed, so I think order of execution of tests or some sort of race condition might play a role in the observed bug, due to some resource which is incorrectly shared between them.

@matthiasbeyer
Copy link
Member Author

Hm... but the tests execute subprocesses and read and write their stdin/stdout... I don't see where are shared resources, tbh.

@petreeftime
Copy link
Collaborator

petreeftime commented Apr 23, 2023

let master_fd = posix_openpt(OFlag::O_RDWR)?;

On the call to posix_openpt, I wonder if the O_NOCTTY flag might be required, the docs seem to imply it changes the master for the calling process.

@petreeftime
Copy link
Collaborator

petreeftime commented Apr 23, 2023

I've tested this and it seems to pass tests on my repo.

https://github.com/petreeftime/rexpect/actions/runs/4779091507

@petreeftime
Copy link
Collaborator

Let me create a PR for the fix.

@petreeftime
Copy link
Collaborator

Attempted fix in #101

Copy link
Collaborator

@ekarlsn ekarlsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach has my blessing. But it seems like more tests have the issue.

This is obviously not how it is done. This patch will be merged to
master so that we can work on master without issues, but a PR will be
filed to revert this patch immediately... with a fix, if someone can
come up with one.

Signed-off-by: Matthias Beyer <[email protected]>
@matthiasbeyer
Copy link
Member Author

Rebased to latest master, to see what's happening.

@matthiasbeyer
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request Aug 17, 2023
@bors
Copy link
Contributor

bors bot commented Aug 17, 2023

try

Build failed:

@matthiasbeyer matthiasbeyer deleted the remove-sporadic-test branch August 17, 2023 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants