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

Print test output properly with --win and --no-win #37

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

DarwinAwardWinner
Copy link

Fixes #24. However, the tests are suboptimal. When running ecukes, stdin isn't a tty, so ert-runner with --no-win doesn't work. I had to use script to trick the emacs subprocess into thinking stdin was a tty in order to get it to work. For the --win option, I couldn't get the test to work at all. It just opened an empty Emacs window and stopped until I quit manually, at which point the test failed. However, when I ran the tests of my own package (ido-ubiquitous) with ert-runner --win, they worked perfectly.

@rejeep
Copy link
Owner

rejeep commented Dec 12, 2016

The tests fail on Travis. From your description it seems that all but the --win should pass? But in Travis, the --no-win fails as well (looks like you are trying to run the script command, but I guess that's not what you want?).

@DarwinAwardWinner
Copy link
Author

The script command is needed for --no-win to work, or else it says something like error: stdin is not a tty.

@rejeep
Copy link
Owner

rejeep commented Dec 12, 2016

What is the script command?

script - make typescript of terminal session

What does "error: stdin is not a tty" even mean in this case? And why does it happen?

@DarwinAwardWinner
Copy link
Author

I don't fully understand what a tty is, but broadly speaking, it means an interactive terminal. If stdin and stdout are redirected to/from a pipe or a file, then it's not a tty, and you can't do things like run an editor because there's no terminal on which to display the interface.

I assume that ecukes is running the tests in a context where stdin and stdout are redirected, so emacs -nw, and almost any other similar program, will give an error about stdin now being a tty.

The script command apparently does some kind of magic to trick the program into thinking that it's running on a tty. Again, I don't understand the details, it's just the answer that I've found, e.g. here. Also, the BSD and util-linux versions of the script command have completely different syntax, and I wrote my code on a Mac, so that's probably why it failed on Travis. Another possible option for this might be socat.

As I said, the tests are suboptimal. I'm not sure if it's possible to get these options working correctly in ecukes.

@rejeep
Copy link
Owner

rejeep commented Jan 15, 2017

Sorry, forgot about this. I don't mind your solution, but the reason I asked before was that the tests does not pass at all.

@DarwinAwardWinner
Copy link
Author

I just don't think it's possible for the --win and --no-win options to be tested within ecukes. They both require an interactive execution environment, not a batch environment. If there's some shell magic to trick them into thinking they're running interactively, I don't know of it.

@rejeep
Copy link
Owner

rejeep commented Jan 15, 2017

Too bad, but let's just ignore the tests then. We have other tests that make sure this won't break anything at least.

@DarwinAwardWinner
Copy link
Author

Ok, so I guess I'll remove the tests for --win and --no-win?

@rejeep
Copy link
Owner

rejeep commented Jan 28, 2017

Yes, let's do so.

This should allow Cask to install the package using Git.
@DarwinAwardWinner
Copy link
Author

I ran into an issue where an error during file loading when running with --win or --no-win would cause the interactive Emacs to remain open rather than exiting. This is obviously bad for use in the context of a batch workflow or continuous integration. I fixed it by wrapping the relevant section of ert-runner in a condition-case that calls kill-emacs on all errors.

@Alexander-Miller
Copy link

Any chance we can get this merged? I currently have a build failing only in CI, so I can really use some proper output now.

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