[Impala-ASF-CR] IMPALA-6582: fix test multiline queries in history
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 ArmstrongGerrit-Reviewer: Alex Behm Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Sat, 24 Feb 2018 22:04:14 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6582: fix test multiline queries in history
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 BehmTested-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 Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-6582: fix test multiline queries in history
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 ArmstrongGerrit-Reviewer: Alex Behm Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Sat, 24 Feb 2018 18:34:32 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6582: fix test multiline queries in history
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 ArmstrongGerrit-Reviewer: Alex Behm Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Sat, 24 Feb 2018 18:18:02 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6582: fix test multiline queries in history
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 ArmstrongGerrit-Reviewer: Alex Behm Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Sat, 24 Feb 2018 18:18:29 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6582: fix test multiline queries in history
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 ArmstrongGerrit-Reviewer: Alex Behm
[Impala-ASF-CR] IMPALA-6582: fix test multiline queries in history
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 ArmstrongGerrit-Reviewer: Alex Behm Gerrit-Comment-Date: Sat, 24 Feb 2018 04:56:48 + Gerrit-HasComments: Yes
[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