FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Remove sporadicly failing test by matthiasbeyer · Pull Request #99 · rust-cli/rexpect · GitHub

/ rexpect Public

Remove sporadicly failing test - #99

Closed
matthiasbeyer wants to merge 1 commit into
rust-cli:masterfrom
matthiasbeyer:remove-sporadic-test
ConversationCommits (1)ChecksFiles changed
Closed

Remove sporadicly failing test#99
matthiasbeyer wants to merge 1 commit into
rust-cli:masterfrom
matthiasbeyer:remove-sporadic-test

Conversation

matthiasbeyer commented Apr 13, 2023

Copy link
Copy Markdown
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 commented Apr 15, 2023

Copy link
Copy Markdown
Member Author

bors merge=petreeftime

bors Bot added a commit that referenced this pull request Apr 15, 2023
Merge #99
30acc9d 
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 <mail@beyermatthias.de>

bors Bot commented Apr 15, 2023

Copy link
Copy Markdown
Contributor

Build failed:

petreeftime commented Apr 23, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator

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 commented Apr 23, 2023

Copy link
Copy Markdown
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 commented Apr 23, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator

rexpect/src/process.rs

Line 89 in 59213cd

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 commented Apr 23, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator

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

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

petreeftime commented Apr 23, 2023

Copy link
Copy Markdown
Collaborator

Let me create a PR for the fix.

petreeftime commented Apr 23, 2023

Copy link
Copy Markdown
Collaborator

Attempted fix in #101

ekarlsn approved these changes May 2, 2023
View reviewed changes

ekarlsn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Remove sporadicly failing test
be7fb7e 
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 <mail@beyermatthias.de>

matthiasbeyer commented Aug 17, 2023

Copy link
Copy Markdown
Member Author

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

matthiasbeyer commented Aug 17, 2023

Copy link
Copy Markdown
Member Author

bors try

bors Bot added a commit that referenced this pull request Aug 17, 2023
Try #99:
330dfcb

bors Bot commented Aug 17, 2023

Copy link
Copy Markdown
Contributor

try

Build failed:

matthiasbeyer mentioned this pull request Aug 17, 2023
Merged
matthiasbeyer deleted the remove-sporadic-test branch August 17, 2023 07:57
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

petreeftime petreeftime approved these changes
ekarlsn ekarlsn approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL