Fixes #15257 - let print adapters decide whether to paginate #243
Conversation
| @@ -328,7 +328,9 @@ def pagination_supported? | |||
| end | |||
|
|
|||
| def should_retrieve_all? | |||
| pagination_supported? && option_per_page.nil? && option_page.nil? && HammerCLI::Settings.get(:ui, :per_page).nil? | |||
| retrieve_all = (pagination_supported? && option_per_page.nil? && option_page.nil?) | |||
| retrieve_all = (retrieve_all && HammerCLI::Settings.get(:ui, :per_page).nil?) if output.adapter.paginate_by_default? | |||
There was a problem hiding this comment.
Just a style thing - it could be shortened to:
retrieve_all &&= HammerCLI::Settings.get(:ui, :per_page).nil? if output.adapter.paginate_by_default?
|
While we're here, do you think 1000 is a good buffer size for any table? It seems a bit big to me. https://github.com/theforeman/hammer-cli-foreman/blob/master/lib/hammer_cli_foreman/commands.rb#L272 |
| end | ||
|
|
||
| def expect_paged_call(page, per_page, item_cnt) | ||
| api_expects(:domains, :index, "List records page #{page}") do |par| |
There was a problem hiding this comment.
All of our other tests which hit the stubbed API are considered functional tests because they test behavior with respect to the API. Usually unit tests test a single method without exercising any of its interfaces. Could you please justify why you've placed this in the unit tests directory?
There was a problem hiding this comment.
I don't have strong preference here but I won't mind leaving it in unit tests as it is tested on abstract command that is not exposed to the user.
There was a problem hiding this comment.
I was actually hesitating where to put it, but chose unit tests in the end for the reason @mbacovsky mentioned - it's testing behaviour aspect of an abstract command the users don't know about (also without checking what's being printed to the output).
I'm ok to move it if you think it's wrong. In fact, I don't care too much about where the tests are placed as long as they exist ;-)
There was a problem hiding this comment.
@tstrachota I'd still call them functional because HammerCLIKatello and other libraries use this abstract class. I mostly noticed the test location because you had to move the API expectation helpers out of the test/functional/test_helper.rb. Also, what about the ListCommand tests that already exist? It's no show stopper for me, though, if you all agree that they're better classified as unit tests.
|
I liked it @tstrachota, thanks for the tests. |
tstrachota
force-pushed
the
output_adapter_pagination_15257
branch
from
e2c32d9
to
9a13984
Compare
|
@akofink regarding the buffer size, you're right that one number probably won't fit all tables, but I'd solve it as a separate issue based on statistics from real world usage rather than guessing the numbers now. |
|
@akofink @mbacovsky thanks for the comments! Updated, some replies inline. |
|
[test] |
1 similar comment
|
[test] |
|
@tstrachota it seems that recent changes in validators theforeman/hammer-cli#207 changed the output message used and it seems it is the reason the tests fail here. |
|
[test] |
|
@mbacovsky a fix has been merged so the tests are green now. I'm going to move the tests and then we can hopefully merge and release 0.7 |
tstrachota
force-pushed
the
output_adapter_pagination_15257
branch
from
8e41b77
to
af3e01f
Compare
tstrachota
force-pushed
the
output_adapter_pagination_15257
branch
from
af3e01f
to
17238dc
Compare
|
@mbacovsky @akofink I moved the tests into functional and merged them with the existing ones. Unfortunately it was extremely slow to check output of the command that printed thousands of records, so I decided to keep those two things separate as "api interaction" and "pagination output". |
|
LGTM! ACK |
|
Thank you, merged |
Requires theforeman/hammer-cli#208