Skip to content
This repository has been archived by the owner on May 14, 2024. It is now read-only.

Generate test specification from a reference program's runs #13

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

Generate test specification from a reference program's runs #13

wants to merge 5 commits into from

Conversation

sahinakkaya
Copy link
Contributor

@sahinakkaya sahinakkaya commented Jul 20, 2020

I tried to implement #9. It works without any problem and benefits from the base classes of calico. It will work with Python 3.6+ because it has f-strings but I didn't want to format them with % because f-strings are not the only problem. textwrap.indent and str.splitlines(keepends=True), which are needed for my implementation, are not available for Python 2. So I decided to not support Python 2 for this feature.

action_type, action_data, timeout = self
timeout = "" if timeout == -1 else f"# timeout: {timeout}"
if action_data != "_EOF_":
action_data = dumps(action_data)
Copy link
Contributor Author

@sahinakkaya sahinakkaya Jul 20, 2020

Choose a reason for hiding this comment

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

I have tried ruamel.yaml.dump but it's not doing what I want while dumping strings:

>>> yaml.dump("6")
"'6'\n"
>>> yaml.dump("str")
'str\n...\n'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is no longer a problem. I'm able to generate the yaml file with ruamel.yaml. I can work on it if this is OK to merge.

calico/base.py Outdated
Comment on lines 385 to 404
def __spawn(self):
pid, master_fd = pty.fork()
if pid == pty.CHILD:
os.execlp(self.argv[0], *self.argv)
try:
mode = tty.tcgetattr(pty.STDIN_FILENO)
tty.setraw(master_fd, termios.TCSANOW)
except tty.error:
restore = False
else:
restore = True

try:
pty._copy(master_fd, self.__read_write_handler, self.__read_write_handler)
except OSError:
if restore:
tty.tcsetattr(pty.STDIN_FILENO, tty.TCSAFLUSH, mode)

os.close(master_fd)
return os.waitpid(pid, 0)[1] >> 8
Copy link
Contributor Author

@sahinakkaya sahinakkaya Aug 2, 2020

Choose a reason for hiding this comment

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

This function is almost exact copy of pty.spawn with a difference on one line: tty.setraw(master_fd, termios.TCSANOW). It puts the newly opened pseudo-terminal into raw mode and I had no idea why this solves the problem I mentioned here. tty.setraw sets some flags for the given file descriptor and only few of them are related with our problem. These are the lines that I found necessary for us after a bit of debugging:

mode[OFLAG] = mode[OFLAG] & ~(OPOST)
mode[LFLAG] = mode[LFLAG] & ~(ECHO | ICANON | IEXTEN | ISIG) # and only `~ECHO` is enough here actually

OPOST: Enable implementation-defined output processing.

This, for example when enabled puts a carriage return, \r, before newline character\n. Since it's negated in the above code, it treats newline as a newline and I had to add it manually afterwards (413th line in this file). So I think we don't need this line. (OPOST will be set, we don't need to add \r manually)

ECHO: Echo input characters.

This will make the pseudo-terminal device to echo it's stdin when enabled. We already capture it through our stdin, so we should disable echoing of the same characters. This line is needed.

The other flags, (ICANON, IEXTEN, ISIG), have no effect on the result as far as I have observed.

I also realized that try ... except ... thing for restoring has no effect because we don't change anything for the file descriptor, pty.STDIN_FILENO. We can also delete these lines.

The purpose of this review was to mention pty.spawn to prove the correctness of my function with a suggestion to delete try ... except ... statement but it ended up explaining all the shady parts. I'll update the code with the changes. Hopefully, it will become much simpler.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant