-
Notifications
You must be signed in to change notification settings - Fork 691
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
Change exitcode-stdio-1.0 description to match current status quo #8604
Conversation
Sandwich certainly looks cool and I see no reason not try supporting it better. AFAIK, the test types are an abandoned project. E.g., the I think you are asking the right questions. To be properly lazy, I'd rather lift the restriction on exitcode-stdio-1.0 than revisit the test types idea, if that can be done in a backward compatible manner. But I have no idea what the purpose of the restriction is. Ease of implementation? Some benefit to users? To other tools? |
Nope! It predates Sandwich by many years.
I'm not sure, but having just delved into Stack I can report that Stack follows Cabal's lead here. I think the tricky part about allowing However, we've just landed a PR in Stack to relax the restriction! commercialhaskell/stack#5951 allows test access to |
My 2¢: the feature makes sense, and I agree with Mikolaj that it's better not to multiply the number of test types, so extending exit-code-1.0 would be preferable. At the same time, it has potential of breaking someone's workflow. I am more of move-fast-break-things person, so I'd embrace this risk. Only, it'd be nice to have a flag either enable or disable the feature; probably, the former is safer, not sure if it satisfies the topic starter. |
We can't be outdone by stack! :D Let's add the flag to cabal as well. Can we play the older, more conservative brother, and have the flag in backward compatible state by default? What are the pros and cons? What are the failure modes? E.g., when somebody runs all tests in parallel and one of them needs stdin, what error is the user going to get? And how would it be recorded in a CI log? And the other way around, if stdin is forbidden and somebody tries to run a test with Sandwitch, what message is going to appear? |
Actually, after some further investigation it seems that the problem with Cabal is the opposite of the problem with Stack.
The actual proposal, then, would be to make [0]: As a result, if a test does a |
However, please consider Regarding cabal not following the |
I feel like Are there any disadvantages to that usage of |
Thank you both for the suggestions, they both work! Both As you can probably tell, I'm not a heavy Cabal user. At this point there's a working solution I can put in the Sandwich docs, so I'm happy. My only remaining concern would be to make starting Sandwich as ergonomic/obvious as possible for Cabal users. I would love if
I'd suggest maybe a text-only change?
could become
|
Yes, please. :) |
@thomasjm could you perhaps update the initial posting and the title of the issue to say that after discussion here you're seeking an improvement in documentation and which one, please? |
Is this documented in the manual? I can't seem to find either |
You are right. Definitely worth a ticket. |
5846f06
to
44f6841
Compare
I've updated the PR title and added a commit for the documentation change we discussed. |
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.
LGTM
@thomasjm: I've given you Triage rights to the repo. Should you accept, you will be able to set the merge_me label that's going to merge this PR after 2 days of no activity. As soon as you are ready. If you see anything related that you'd like to fix or anything that would help consumers of Sandwich, don't hesitate to open tickets. :) |
Will do, thanks! |
@thomasjm we do merges via rebases, so our merge bot needs permission to update your branch, as indicated by this error message here:
Could you fix it? The way to switch is described here, I think. |
44f6841
to
09f9a3c
Compare
Ah I don't have that checkbox since my fork is on an organization repo. I can re-open the PR from a personal repo if necessary. I just hit the rebase button myself so maybe that will work? |
It may help but I doubt it, let's wait for updated mergify status. In the past people had to reopen PR in such cases. |
Oh, but it did work! Thanks a lot for your contribution, @thomasjm! |
Hi, I'm hoping to solicit comments/design discussion on an idea to add a new test suite interface.
I'm the author of the Sandwich test framework, which provides a way of running and visualizing tests in a terminal UI interface. It would be nice to add first-class support for this way of working to tooling like Cabal, so you could type
cabal test
from your terminal and get dropped into the interface.This is currently not possible because Cabal's existing test suite types don't allow the tests to use
stdin
; see the exitcode-stdio-1.0 documentation.This PR proposes to add a new
TestSuiteInterface
constructor calledTestSuiteInteractiveV10
, which behaves just likeTestSuiteExeV10
except for the restriction onstdin
. I haven't filled in any other code and I'm not sure how hard the rest of the implementation would be.Possible alternative: simply change the semantics of
exitcode-stdio-1.0
to allowstdin
when used in interactive settings. It might be painful to add a new test suite type, because most tests in the wild assumeexitcode-stdio-1.0
(and some tools likehpack
currently bake it in by default).This PR was emboldened by @Mikolaj 's email to Haskell-Cafe :)
Please include the following checklist in your PR: