You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tim Armstrong (Code Review)" <ge...@cloudera.org> on 2018/02/24 01:58:01 UTC
[Impala-ASF-CR] IMPALA-6582: fix test multiline queries in history
Tim Armstrong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9441
Change subject: IMPALA-6582: fix test_multiline_queries_in_history
......................................................................
IMPALA-6582: fix test_multiline_queries_in_history
The semicolon was in the wrong place in one of the test queries and
the failure was swallowed silently.
This meant that one fewer prompt was displayed than expected. This
didn't cause a test failure because the prompt regex also matched the
"Connected to host:port" message printed in the shell preamble. I'm
unsure why this would cause the test failure but my best theory is that
in the failure case, the "Connected" and prompt messages are both
buffered when we evaluate the first prompt regex, and the regex swallows
up the whole input, rather than just the first instance.
Testing:
Tightened up the prompt regex and checked that the query actually
executed successfully. With these improvements, the broken query
text caused a test failure.
I looped the test for a while to make sure it was robust.
Change-Id: If917bbc8e87b83c188b6d5e1acad912892b8c6fe
---
M tests/shell/test_shell_interactive.py
1 file changed, 3 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/9441/1
--
To view, visit http://gerrit.cloudera.org:8080/9441
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If917bbc8e87b83c188b6d5e1acad912892b8c6fe
Gerrit-Change-Number: 9441
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-6582: fix test multiline queries in history
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9441 )
Change subject: IMPALA-6582: fix test_multiline_queries_in_history
......................................................................
IMPALA-6582: fix test_multiline_queries_in_history
The semicolon was in the wrong place in one of the test queries and
the failure was swallowed silently.
This meant that one fewer prompt was displayed than expected. This
didn't cause a test failure because the prompt regex also matched the
"Connected to host:port" message printed in the shell preamble. I'm
unsure why this would cause the test failure but my best theory is that
in the failure case, the "Connected" and prompt messages are both
buffered when we evaluate the first prompt regex, and the regex swallows
up the whole input, rather than just the first instance.
Testing:
Tightened up the prompt regex and checked that the query actually
executed successfully. With these improvements, the broken query
text caused a test failure.
I looped the test for a while to make sure it was robust.
Added a couple of related test cases to make sure we aren't losing
coverage.
Change-Id: If917bbc8e87b83c188b6d5e1acad912892b8c6fe
Reviewed-on: http://gerrit.cloudera.org:8080/9441
Reviewed-by: Alex Behm <al...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M tests/shell/test_shell_interactive.py
1 file changed, 14 insertions(+), 7 deletions(-)
Approvals:
Alex Behm: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/9441
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: If917bbc8e87b83c188b6d5e1acad912892b8c6fe
Gerrit-Change-Number: 9441
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-6582: fix test multiline queries in history
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9441 )
Change subject: IMPALA-6582: fix test_multiline_queries_in_history
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9441/1/tests/shell/test_shell_interactive.py
File tests/shell/test_shell_interactive.py:
http://gerrit.cloudera.org:8080/#/c/9441/1/tests/shell/test_shell_interactive.py@220
PS1, Line 220: queries = ["select\n1;--comment",
Hard to say what the original intent of this query was. I would have assumed that the comment should be part of this query like in the other two queries below, i.e. I feel this might have been intended:
"select\n1--comment\n;"
Of course, I don't know for sure. What do you think?
--
To view, visit http://gerrit.cloudera.org:8080/9441
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If917bbc8e87b83c188b6d5e1acad912892b8c6fe
Gerrit-Change-Number: 9441
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Comment-Date: Sat, 24 Feb 2018 04:56:48 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6582: fix test multiline queries in history
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has removed Fredy Wijaya from this change. ( http://gerrit.cloudera.org:8080/9441 )
Change subject: IMPALA-6582: fix test_multiline_queries_in_history
......................................................................
Removed reviewer Fredy Wijaya.
--
To view, visit http://gerrit.cloudera.org:8080/9441
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: If917bbc8e87b83c188b6d5e1acad912892b8c6fe
Gerrit-Change-Number: 9441
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-6582: fix test multiline queries in history
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9441 )
Change subject: IMPALA-6582: fix test_multiline_queries_in_history
......................................................................
Patch Set 2: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/9441
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If917bbc8e87b83c188b6d5e1acad912892b8c6fe
Gerrit-Change-Number: 9441
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 24 Feb 2018 22:04:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6582: fix test multiline queries in history
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9441
to look at the new patch set (#2).
Change subject: IMPALA-6582: fix test_multiline_queries_in_history
......................................................................
IMPALA-6582: fix test_multiline_queries_in_history
The semicolon was in the wrong place in one of the test queries and
the failure was swallowed silently.
This meant that one fewer prompt was displayed than expected. This
didn't cause a test failure because the prompt regex also matched the
"Connected to host:port" message printed in the shell preamble. I'm
unsure why this would cause the test failure but my best theory is that
in the failure case, the "Connected" and prompt messages are both
buffered when we evaluate the first prompt regex, and the regex swallows
up the whole input, rather than just the first instance.
Testing:
Tightened up the prompt regex and checked that the query actually
executed successfully. With these improvements, the broken query
text caused a test failure.
I looped the test for a while to make sure it was robust.
Added a couple of related test cases to make sure we aren't losing
coverage.
Change-Id: If917bbc8e87b83c188b6d5e1acad912892b8c6fe
---
M tests/shell/test_shell_interactive.py
1 file changed, 14 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/9441/2
--
To view, visit http://gerrit.cloudera.org:8080/9441
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If917bbc8e87b83c188b6d5e1acad912892b8c6fe
Gerrit-Change-Number: 9441
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
[Impala-ASF-CR] IMPALA-6582: fix test multiline queries in history
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9441 )
Change subject: IMPALA-6582: fix test_multiline_queries_in_history
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9441/1/tests/shell/test_shell_interactive.py
File tests/shell/test_shell_interactive.py:
http://gerrit.cloudera.org:8080/#/c/9441/1/tests/shell/test_shell_interactive.py@220
PS1, Line 220: queries = ["select\n1;--comment",
> Hard to say what the original intent of this query was. I would have assume
I don't know. I think the new test makes sense, since the comment is included in the history still. Anyway, I added a couple more variations to be sure.
I discovered in the process that trailing newlines are stripped as well, so I had to distinguish the input and expected output. The removal of newlines seems a bit wonky but benign.
--
To view, visit http://gerrit.cloudera.org:8080/9441
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If917bbc8e87b83c188b6d5e1acad912892b8c6fe
Gerrit-Change-Number: 9441
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 24 Feb 2018 18:18:02 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6582: fix test multiline queries in history
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9441 )
Change subject: IMPALA-6582: fix test_multiline_queries_in_history
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9441
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If917bbc8e87b83c188b6d5e1acad912892b8c6fe
Gerrit-Change-Number: 9441
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 24 Feb 2018 18:34:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6582: fix test multiline queries in history
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9441 )
Change subject: IMPALA-6582: fix test_multiline_queries_in_history
......................................................................
Patch Set 2:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2010/
--
To view, visit http://gerrit.cloudera.org:8080/9441
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If917bbc8e87b83c188b6d5e1acad912892b8c6fe
Gerrit-Change-Number: 9441
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 24 Feb 2018 18:18:29 +0000
Gerrit-HasComments: No