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

Repl integration testing #398

Merged
merged 2 commits into from
Apr 8, 2019
Merged

Repl integration testing #398

merged 2 commits into from
Apr 8, 2019

Conversation

se7entyse7en
Copy link
Contributor

@se7entyse7en se7entyse7en commented Apr 2, 2019

Closes #334.

I've tested many other possibilities, but I failed with each of them. All the problems derived from two things:

  1. make the stdin be recognized as a char mode device here,
  2. keep the stdin open so that I can write queries interactively.
  • I've tried running the command as /bin/bash -e <path to srcd> sql, but it was exiting as if it was run normally,
  • I've tried using os.Stdin as command.Stdin, but it has a couple of problems:
    1. the stdin was correctly recognized as char mode device (obviously),
    2. if I don't write on command.Stdin before starting the command, then stdin was interpreted as closed (or reads EOF) so it just exits immediately
  • I've then tried to use a Reader (command.Stdin is a Reader) which is channel-based so that stdin is not interpreted as closed as long as I don't close the channel. This was actually both a Reader and a Writer so that it is used as a Reader by the program and as a Writer by the test to write queries:
    1. the stdin was not interpreted as closed, so I was able to write queries after the command has been started,
    2. the stdin was not recognized as a char mode device, so it was entering in this if,
    3. so what I was writing was actually interpreted as if the input was piped in.
  • I've tried using a bytes.Buffer, but it has the same effect, but it was also additionally not recognized as a char mode device.
  • I've then tried to improve the previous solution and tried to hack that first read by sending an EOF without closing the channel. I was able to implement it by configuring the Reader to send an EOF when I write a specificied marker (in the previous solution the EOF was being sent only when the underlying channel was closed):
    1. it was hacky, and it wasn't working, I don't even remember what exactly...

Then I discovered that I could actually run a pseudo-terminal. By using a pseudo-terminal the stdin was correctly interpreted as a char mode device, and it was not interpreted as closed immediately.

cmd/srcd/cmd/sql_test.go Outdated Show resolved Hide resolved
cmd/srcd/cmd/sql_test.go Outdated Show resolved Hide resolved
cmd/srcd/cmd/sql_test.go Outdated Show resolved Hide resolved
cmd/srcd/cmd/sql_test.go Outdated Show resolved Hide resolved
cmd/srcd/cmd/sql_test.go Outdated Show resolved Hide resolved
cmd/srcd/cmd/sql_test.go Outdated Show resolved Hide resolved
cmd/srcd/cmd/sql_test.go Outdated Show resolved Hide resolved
cmd/srcd/cmd/sql_test.go Outdated Show resolved Hide resolved
cmd/srcd/cmd/sql_test.go Outdated Show resolved Hide resolved
cmd/srcd/cmd/sql_test.go Outdated Show resolved Hide resolved
@se7entyse7en
Copy link
Contributor Author

I've just realized that I haven't pushed on my fork.

@se7entyse7en
Copy link
Contributor Author

@smacker all issues you pointed out should have been addressed here. I've extracted a StreamLinifier util that is responsible of yielding lines from a stream of strings.

cmd/srcd/cmd/sql_test.go Outdated Show resolved Hide resolved
cmd/srcd/cmd/sql_test.go Outdated Show resolved Hide resolved
cmd/test-utils/common.go Outdated Show resolved Hide resolved
Copy link
Contributor

@smacker smacker left a comment

Choose a reason for hiding this comment

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

The only thing I worry about is when sql doesn't return results or returns error. But it's not very important.

cmd/srcd/cmd/sql_test.go Outdated Show resolved Hide resolved
cmd/srcd/cmd/sql_test.go Outdated Show resolved Hide resolved
cmd/srcd/cmd/sql_test.go Outdated Show resolved Hide resolved
@se7entyse7en
Copy link
Contributor Author

Gonna fix DCO before rebasing and merging.

Signed-off-by: Lou Marvin Caraig <[email protected]>
Signed-off-by: Lou Marvin Caraig <[email protected]>
@se7entyse7en se7entyse7en force-pushed the repl-integration-testing branch from 5104ed7 to 791ee07 Compare April 5, 2019 17:34
@se7entyse7en
Copy link
Contributor Author

se7entyse7en commented Apr 5, 2019

Rebased and squashed. I've also added some docs for the StreamLinifier.

Side note: we currently cannot use icmd for testing the interactive SQL REPL. See here. I've created an issue.

@se7entyse7en se7entyse7en merged commit d12e19a into master Apr 8, 2019
@se7entyse7en se7entyse7en deleted the repl-integration-testing branch April 8, 2019 09:51
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.

Test for interactive sql REPL
4 participants