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

Adding a test for the scalding repl #890

Merged
merged 9 commits into from
Jun 12, 2014
Merged

Conversation

sriramkrishnan
Copy link
Collaborator

Also includes some refactoring of the shell scripts to avoid duplication of code.

@johnynek
Copy link
Collaborator

johnynek commented Jun 7, 2014

Nice. @ianoc is the local bash-master. I am not competent to review this. Glad to have the tests.

@@ -51,3 +59,14 @@ $SCALD tutorial/MatrixTutorial5.scala \
$SCALD --json tutorial/JsonTutorial0.scala

$SCALD --avro --json tutorial/AvroTutorial0.scala

# Now run a basic test for the REPL
# If the content of the output is different, diff will fail (exit code 1)
Copy link

Choose a reason for hiding this comment

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

The returned exit code is actually still 127, as set in the commons script.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe the comment wasn't clear. But "diff tutorial/data/hello.txt tutorial/data/output1.txt" exits with a non-zero exit code, and hence the test_tutorials.sh fails will fail (due to the set -e). Which is what we want. I will clarify the comment.

@hougs
Copy link

hougs commented Jun 9, 2014

I think this test looks great, thanks for adding it. The code that you factored into the script commons is useful for the repl, because otherwise it doesn't exit correctly when you try to quit it. I'm not sure what the use is in the test script. I would recommend getting rid of that if it doesn't have a purpose. You shouldn't need it to run the repl from a user's POV. I think it actually just makes it harder to understand what is failing and where, because of the rigamaroll w exit statuses.
Thanks for the good work!

@sriramkrishnan
Copy link
Collaborator Author

If I don't use the common code, running the test_tutorials.sh from the command-line makes you lose your stty settings. And it is reasonable to expect that one would want to run the test tutorials script from the command-line (e.g. when developing locally). So I wanted to make sure that we have the same code there. I factored it out so I didn't have to duplicate the same code. But yeah, refactoring bash code and preserving exit statuses doesn't necessarily make it easier to read. In this case, we just want to exit with a non-zero exit status though if any of the tutorials fails.

bin=`dirname $0`
bin=`cd ${bin}/.. && pwd`
Copy link
Collaborator

Choose a reason for hiding this comment

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

dirname $0 can fail to identify the current working path for the distro under relative paths or other things... we should use

DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"

for it, its about the most reliable way to get the info

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do. Updated PR coming.

@sriramkrishnan
Copy link
Collaborator Author

So @ianoc, any more comments? Can this be merged? Thanks!

ianoc added a commit that referenced this pull request Jun 12, 2014
Adding a test for the scalding repl
@ianoc ianoc merged commit b601888 into twitter:develop Jun 12, 2014
@sriramkrishnan
Copy link
Collaborator Author

Thanks all!

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.

None yet

4 participants