-
Notifications
You must be signed in to change notification settings - Fork 12
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
cat/play: tip that multiple files can be processed #1029
base: master
Are you sure you want to change the base?
cat/play: tip that multiple files can be processed #1029
Conversation
a0b1cb8
to
95cb73e
Compare
I also reformatted the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just 2 NITs for your discretion.
95cb73e
to
124f1be
Compare
test/integration/cat/test_cat.py
Outdated
assert re.search(r"lsn: 512", output) | ||
assert re.search(r"space_id: 320", output) | ||
assert re.search(r"space_id: 296", output) | ||
def make_test_cat_args_param( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: what is the benefit of this extra layer of abstraction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We set default values for all parameters, and could skip them in some test cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there cases where a parameter can be omitted and default values used instead?
Even so, it is more transparent to have an explicit indication that the input is “empty” data than to do so implicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there cases where a parameter can be omitted and default values used instead?
Yes. I removed this code from places, where it does not needed.
test/integration/cat/test_cat.py
Outdated
|
||
|
||
def test_cat_xlog_file(tt_cmd, tmp_path): | ||
@pytest.mark.parametrize(TEST_CAT_ARGS_CCONFIG, [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to hide a set of arguments in the variable? It doesn't seem transparent to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I found the case, when two sets of the parameters are used. I just decided not to delete this. Maybe it makes the code overloaded, I agree. Do you think I should remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- As for me, it makes the code a bit difficult to understand.
- It complicates the ability to make local changes, as there is an unnecessary binding to a variable that can be used elsewhere, and you need to maintain consistency across different places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
test/integration/cat/test_cat.py
Outdated
@pytest.mark.parametrize(TEST_CAT_ARGS_CCONFIG, [ | ||
make_test_cat_args_param( | ||
# Testing with unset .xlog or .snap file. | ||
cat_result=1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I understand correctly that cat_result
is the return code of the external utility?
It looks like we are testing the operation of this utility. It should be enough to check whether it succeeds or not, and with what code it doesn't matter.
Therefore, it would be more correct to split the test - one for successful cases, the other for error cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return code of the external utility?
Nope, this is result of tt cat <args>
command execution.
split the test
Now they divided to common
part and timestamp
testing part. I thought that a good idea not to mix them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right about the “good idea not to mix them”, that's the reason why it's better to stick with that idea to the end and separate them into successful and unsuccessful ones as well.
So it would be something like:
- common success
- common fail
- timestamp success
- timestamp fail
This separation will make the code simpler, remove unnecessary parameterization at the input and unnecessary flow control (if-else) inside the test function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I divided tests by arguments and results.
test/integration/cat/test_cat.py
Outdated
assert re.search(r"space_id: 320", output) | ||
assert re.search(r"space_id: 296", output) | ||
def make_test_cat_args_param( | ||
args=[], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Python is dangerous the use a mutable data types for the default value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, my mistake. Fixed.
124f1be
to
c7ada0e
Compare
c7ada0e
to
7795e70
Compare
@TarantoolBot document
Title: tip that multiple files can be processed by
tt cat
andtt play
This patch updates help of the
cat
andplay
commands to highlights that multiple files can be processed.Closes #1018
Closes #1019