You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Thomas Marshall (Code Review)" <ge...@cloudera.org> on 2019/04/08 21:38:01 UTC
[Impala-ASF-CR] IMPALA-8390: clean up test vectors in test cancellation.py
Thomas Marshall has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12960
Change subject: IMPALA-8390: clean up test vectors in test_cancellation.py
......................................................................
IMPALA-8390: clean up test vectors in test_cancellation.py
Due to changes to TestCancellation made in IMPALA-7205 that were not
reflected in TestCancellationSerial and TestCancellationFullSort,
test_cancel_insert has not been running at all and test_cancel_sort
has been running with unintended parameters.
This patch re-enables test_cancel_insert, while including a number of
constraints on its parameters to keep test execution time reasonable.
It also fixes an incorrect constraint on test_cancel_sort.
The patch also makes some related improvements:
- Removes an xfail on test_cancel_insert related to a bug that is
fixed now.
- When ImpalaTestVector.get_value() is called with a value name that
does not actually exist in the vector, the result is a StopIteration
exception. Due to python's questionable habit of using exceptions
for flow control, StopIteration is frequently treated not as an
error but as the normal end of iteration, which can result in
unexpected behavior, eg. when pytest_generate_tests raises a
StopIteration pytest just silently ignores it and drops the test
case. This patch modifies get_value() to instead raise a ValueError
in this situation.
- When a test has no vectors generated for it, the name of the test is
now included in the logged warning.
Change-Id: I9673fe82bda5314aff6a51d1961759ff286fbf6f
---
M tests/common/test_vector.py
M tests/conftest.py
M tests/query_test/test_cancellation.py
3 files changed, 27 insertions(+), 14 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/12960/1
--
To view, visit http://gerrit.cloudera.org:8080/12960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9673fe82bda5314aff6a51d1961759ff286fbf6f
Gerrit-Change-Number: 12960
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-8390: clean up test vectors in test cancellation.py
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/12960 )
Change subject: IMPALA-8390: clean up test vectors in test_cancellation.py
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12960/2/tests/common/test_vector.py
File tests/common/test_vector.py:
http://gerrit.cloudera.org:8080/#/c/12960/2/tests/common/test_vector.py@79
PS2, Line 79:
> Ugh. There were a few copy/paste errors in that code snippet above, but you
Agreed a for loop is much more readable. Updated
--
To view, visit http://gerrit.cloudera.org:8080/12960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9673fe82bda5314aff6a51d1961759ff286fbf6f
Gerrit-Change-Number: 12960
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 12 Apr 2019 17:49:09 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8390: clean up test vectors in test cancellation.py
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12960 )
Change subject: IMPALA-8390: clean up test vectors in test_cancellation.py
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9673fe82bda5314aff6a51d1961759ff286fbf6f
Gerrit-Change-Number: 12960
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 12 Apr 2019 19:23:25 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8390: clean up test vectors in test cancellation.py
Posted by "David Knupp (Code Review)" <ge...@cloudera.org>.
David Knupp has posted comments on this change. ( http://gerrit.cloudera.org:8080/12960 )
Change subject: IMPALA-8390: clean up test vectors in test_cancellation.py
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12960/2/tests/common/test_vector.py
File tests/common/test_vector.py:
http://gerrit.cloudera.org:8080/#/c/12960/2/tests/common/test_vector.py@79
PS2, Line 79: ValueError
> Pedantic nit: ValueError is probably the wrong thing to raise here. From ht
Actually -- wait. I don't know what I was misreading before. ValueError is exactly the right exception to raise. Sorry about that.
That said, I still think a for/else loop is more idiomatic.
--
To view, visit http://gerrit.cloudera.org:8080/12960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9673fe82bda5314aff6a51d1961759ff286fbf6f
Gerrit-Change-Number: 12960
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 12 Apr 2019 00:10:33 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8390: clean up test vectors in test cancellation.py
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12960 )
Change subject: IMPALA-8390: clean up test vectors in test_cancellation.py
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2757/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/12960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9673fe82bda5314aff6a51d1961759ff286fbf6f
Gerrit-Change-Number: 12960
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 12 Apr 2019 18:28:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8390: clean up test vectors in test cancellation.py
Posted by "David Knupp (Code Review)" <ge...@cloudera.org>.
David Knupp has posted comments on this change. ( http://gerrit.cloudera.org:8080/12960 )
Change subject: IMPALA-8390: clean up test vectors in test_cancellation.py
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9673fe82bda5314aff6a51d1961759ff286fbf6f
Gerrit-Change-Number: 12960
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 12 Apr 2019 19:18:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8390: clean up test vectors in test cancellation.py
Posted by "David Knupp (Code Review)" <ge...@cloudera.org>.
David Knupp has posted comments on this change. ( http://gerrit.cloudera.org:8080/12960 )
Change subject: IMPALA-8390: clean up test vectors in test_cancellation.py
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12960/2/tests/common/test_vector.py
File tests/common/test_vector.py:
http://gerrit.cloudera.org:8080/#/c/12960/2/tests/common/test_vector.py@79
PS2, Line 79: ValueError
> Actually -- wait. I don't know what I was misreading before. ValueError is
Ugh. There were a few copy/paste errors in that code snippet above, but you get the idea.
Can I assume that vector_values is just a list or tuple of things -- not an actual iterator object?
--
To view, visit http://gerrit.cloudera.org:8080/12960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9673fe82bda5314aff6a51d1961759ff286fbf6f
Gerrit-Change-Number: 12960
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 12 Apr 2019 03:33:56 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8390: clean up test vectors in test cancellation.py
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12960 )
Change subject: IMPALA-8390: clean up test vectors in test_cancellation.py
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12960/1/tests/common/test_vector.py
File tests/common/test_vector.py:
http://gerrit.cloudera.org:8080/#/c/12960/1/tests/common/test_vector.py@76
PS1, Line 76: \
flake8: E502 the backslash is redundant between brackets
http://gerrit.cloudera.org:8080/#/c/12960/1/tests/common/test_vector.py@79
PS1, Line 79: ,
flake8: W602 deprecated form of raising exception
--
To view, visit http://gerrit.cloudera.org:8080/12960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9673fe82bda5314aff6a51d1961759ff286fbf6f
Gerrit-Change-Number: 12960
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Mon, 08 Apr 2019 21:38:52 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8390: clean up test vectors in test cancellation.py
Posted by "David Knupp (Code Review)" <ge...@cloudera.org>.
David Knupp has posted comments on this change. ( http://gerrit.cloudera.org:8080/12960 )
Change subject: IMPALA-8390: clean up test vectors in test_cancellation.py
......................................................................
Patch Set 2:
(1 comment)
Looks good to me
http://gerrit.cloudera.org:8080/#/c/12960/2/tests/common/test_vector.py
File tests/common/test_vector.py:
http://gerrit.cloudera.org:8080/#/c/12960/2/tests/common/test_vector.py@79
PS2, Line 79: ValueError
Pedantic nit: ValueError is probably the wrong thing to raise here. From https://docs.python.org/2/library/exceptions.html, a ValueError applies when "an operation or function receives an argument that has the right type but an inappropriate value." I'd probably raise the more generic RuntimeError. (Note to self: submit a patch to python core for a new WtfError class.)
You can also argue that using next() in this instance is kinda weird (it is) and pointlessly clever. The original intent is to stop iterating over vector_values when the desired value is found, but to do it in a one-liner. How about a more verbose but way more legible for/else loop?
for vector_values in self.vector_values:
if vector_value.name == name:
return vector_value.name # stops iteration
else:
do something else if not found
Regardless, I assume that there's no code that actually catches this anywhere? We just want things to blow up noisily, rather than silently failing as with StopIteration.
--
To view, visit http://gerrit.cloudera.org:8080/12960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9673fe82bda5314aff6a51d1961759ff286fbf6f
Gerrit-Change-Number: 12960
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Thu, 11 Apr 2019 23:57:18 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8390: clean up test vectors in test cancellation.py
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/12960 )
Change subject: IMPALA-8390: clean up test vectors in test_cancellation.py
......................................................................
IMPALA-8390: clean up test vectors in test_cancellation.py
Due to changes to TestCancellation made in IMPALA-7205 that were not
reflected in TestCancellationSerial and TestCancellationFullSort,
test_cancel_insert has not been running at all and test_cancel_sort
has been running with unintended parameters.
This patch re-enables test_cancel_insert, while including a number of
constraints on its parameters to keep test execution time reasonable.
It also fixes an incorrect constraint on test_cancel_sort.
The patch also makes some related improvements:
- Removes an xfail on test_cancel_insert related to a bug that is
fixed now.
- When ImpalaTestVector.get_value() is called with a value name that
does not actually exist in the vector, the result is a StopIteration
exception. Due to python's questionable habit of using exceptions
for flow control, StopIteration is frequently treated not as an
error but as the normal end of iteration, which can result in
unexpected behavior, eg. when pytest_generate_tests raises a
StopIteration pytest just silently ignores it and drops the test
case. This patch modifies get_value() to instead raise a ValueError
in this situation.
- When a test has no vectors generated for it, the name of the test is
now included in the logged warning.
Testing:
- Ran full core and exhaustive runs and verified that the expected
test cases are run for test_cancellation.py now
Change-Id: I9673fe82bda5314aff6a51d1961759ff286fbf6f
Reviewed-on: http://gerrit.cloudera.org:8080/12960
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M tests/common/test_vector.py
M tests/conftest.py
M tests/query_test/test_cancellation.py
3 files changed, 26 insertions(+), 14 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/12960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I9673fe82bda5314aff6a51d1961759ff286fbf6f
Gerrit-Change-Number: 12960
Gerrit-PatchSet: 5
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-8390: clean up test vectors in test cancellation.py
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Hello David Knupp, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12960
to look at the new patch set (#2).
Change subject: IMPALA-8390: clean up test vectors in test_cancellation.py
......................................................................
IMPALA-8390: clean up test vectors in test_cancellation.py
Due to changes to TestCancellation made in IMPALA-7205 that were not
reflected in TestCancellationSerial and TestCancellationFullSort,
test_cancel_insert has not been running at all and test_cancel_sort
has been running with unintended parameters.
This patch re-enables test_cancel_insert, while including a number of
constraints on its parameters to keep test execution time reasonable.
It also fixes an incorrect constraint on test_cancel_sort.
The patch also makes some related improvements:
- Removes an xfail on test_cancel_insert related to a bug that is
fixed now.
- When ImpalaTestVector.get_value() is called with a value name that
does not actually exist in the vector, the result is a StopIteration
exception. Due to python's questionable habit of using exceptions
for flow control, StopIteration is frequently treated not as an
error but as the normal end of iteration, which can result in
unexpected behavior, eg. when pytest_generate_tests raises a
StopIteration pytest just silently ignores it and drops the test
case. This patch modifies get_value() to instead raise a ValueError
in this situation.
- When a test has no vectors generated for it, the name of the test is
now included in the logged warning.
Change-Id: I9673fe82bda5314aff6a51d1961759ff286fbf6f
---
M tests/common/test_vector.py
M tests/conftest.py
M tests/query_test/test_cancellation.py
3 files changed, 27 insertions(+), 14 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/12960/2
--
To view, visit http://gerrit.cloudera.org:8080/12960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9673fe82bda5314aff6a51d1961759ff286fbf6f
Gerrit-Change-Number: 12960
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-8390: clean up test vectors in test cancellation.py
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12960 )
Change subject: IMPALA-8390: clean up test vectors in test_cancellation.py
......................................................................
Patch Set 4:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4014/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/12960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9673fe82bda5314aff6a51d1961759ff286fbf6f
Gerrit-Change-Number: 12960
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 12 Apr 2019 19:23:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8390: clean up test vectors in test cancellation.py
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/12960 )
Change subject: IMPALA-8390: clean up test vectors in test_cancellation.py
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12960/1/tests/common/test_vector.py
File tests/common/test_vector.py:
http://gerrit.cloudera.org:8080/#/c/12960/1/tests/common/test_vector.py@76
PS1, Line 76:
> flake8: E502 the backslash is redundant between brackets
Done
http://gerrit.cloudera.org:8080/#/c/12960/1/tests/common/test_vector.py@79
PS1, Line 79: (
> flake8: W602 deprecated form of raising exception
Done
--
To view, visit http://gerrit.cloudera.org:8080/12960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9673fe82bda5314aff6a51d1961759ff286fbf6f
Gerrit-Change-Number: 12960
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 08 Apr 2019 21:59:18 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8390: clean up test vectors in test cancellation.py
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12960 )
Change subject: IMPALA-8390: clean up test vectors in test_cancellation.py
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2685/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/12960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9673fe82bda5314aff6a51d1961759ff286fbf6f
Gerrit-Change-Number: 12960
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Mon, 08 Apr 2019 21:54:12 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8390: clean up test vectors in test cancellation.py
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Hello David Knupp, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12960
to look at the new patch set (#3).
Change subject: IMPALA-8390: clean up test vectors in test_cancellation.py
......................................................................
IMPALA-8390: clean up test vectors in test_cancellation.py
Due to changes to TestCancellation made in IMPALA-7205 that were not
reflected in TestCancellationSerial and TestCancellationFullSort,
test_cancel_insert has not been running at all and test_cancel_sort
has been running with unintended parameters.
This patch re-enables test_cancel_insert, while including a number of
constraints on its parameters to keep test execution time reasonable.
It also fixes an incorrect constraint on test_cancel_sort.
The patch also makes some related improvements:
- Removes an xfail on test_cancel_insert related to a bug that is
fixed now.
- When ImpalaTestVector.get_value() is called with a value name that
does not actually exist in the vector, the result is a StopIteration
exception. Due to python's questionable habit of using exceptions
for flow control, StopIteration is frequently treated not as an
error but as the normal end of iteration, which can result in
unexpected behavior, eg. when pytest_generate_tests raises a
StopIteration pytest just silently ignores it and drops the test
case. This patch modifies get_value() to instead raise a ValueError
in this situation.
- When a test has no vectors generated for it, the name of the test is
now included in the logged warning.
Testing:
- Ran full core and exhaustive runs and verified that the expected
test cases are run for test_cancellation.py now
Change-Id: I9673fe82bda5314aff6a51d1961759ff286fbf6f
---
M tests/common/test_vector.py
M tests/conftest.py
M tests/query_test/test_cancellation.py
3 files changed, 26 insertions(+), 14 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/12960/3
--
To view, visit http://gerrit.cloudera.org:8080/12960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9673fe82bda5314aff6a51d1961759ff286fbf6f
Gerrit-Change-Number: 12960
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-8390: clean up test vectors in test cancellation.py
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12960 )
Change subject: IMPALA-8390: clean up test vectors in test_cancellation.py
......................................................................
Patch Set 4: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/12960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9673fe82bda5314aff6a51d1961759ff286fbf6f
Gerrit-Change-Number: 12960
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Sat, 13 Apr 2019 00:27:18 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8390: clean up test vectors in test cancellation.py
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12960 )
Change subject: IMPALA-8390: clean up test vectors in test_cancellation.py
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2687/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/12960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9673fe82bda5314aff6a51d1961759ff286fbf6f
Gerrit-Change-Number: 12960
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 08 Apr 2019 22:40:53 +0000
Gerrit-HasComments: No