-
-
Notifications
You must be signed in to change notification settings - Fork 306
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
Resolves issue where tests would not attempt to complete #236
base: master
Are you sure you want to change the base?
Conversation
I don't see this being addressed, so here is a workaround I'm using: https://gist.github.com/KoryNunn/a7ddbfb069bbbb2a41b4 |
This needs rebasing, but it's still worth evaluating I think? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
61dde8b
to
ea17937
Compare
@KoryNunn this has been rebased, but tests appear to be failing for me - |
Looks like the changes break other tests, and after 3 years I'm not motivated enough to do anything about it. Doesn't look like it has much requirement since no one else has been involved in this PR. Someone else can have a go if they want. |
I had a look at what the issue was and only the skip test fails. It looks like the outer tap ends before the tape test gets a chance to end it's output stream. I am not sure how to get around that. |
5a483be
to
76ca27f
Compare
Codecov Report
@@ Coverage Diff @@
## master #236 +/- ##
==========================================
- Coverage 74.15% 74.05% -0.10%
==========================================
Files 19 19
Lines 766 771 +5
Branches 146 146
==========================================
+ Hits 568 571 +3
- Misses 198 200 +2
Continue to review full report at Codecov.
|
c16dde3
to
2ad86d4
Compare
This is a solution to the same problem that #235 addresses.
I reckon this is a better solution, but I'll leave them both open for review for now.
The new test fails in the current version of tape, and passes with the changes proposed.