Autocomplete improvements #229
Conversation
vbaltrusaitis-reef
force-pushed
the
autocomplete-improvements
branch
from
5302ed0 to
28694e2
Compare
| @@ -0,0 +1,274 @@ | |||
| ###################################################################### | |||
There was a problem hiding this comment.
it seems to me like this belongs to test/unit since it doesn't require neither shell nor API access
(the API access being crucial thing which makes tests "integration" tests and why we ran them separately and more sparingly)
There was a problem hiding this comment.
No, several tests here do use the API - those that test autocomplete for b2 get-bucket, b2 download-file b2://, etc. Other tests are very similar in structure and are just one step away from using the API the same way. I didn't think that creating a separate test/common package, extracting the common helpers were worth it given the similarity of the integration and non-integration tests here.
Though if you still think that this should be done, I'll do it.
There was a problem hiding this comment.
here is why I think these tests are different:
With other autocomplete tests, we are not mocking anything and spawning a real process - this is why they call real API - because there is no mocking ability then (or would be hard to implement) - these test clearly require ability to mock , therefore, there is no need to use real API.
Integration tests are costly and much hard to maintain, this is why I think it is worth some time to investigate alternative.
Seeing how you implemented your own runner it seems to me that if you started in unit test folder you would have the same amount of work to do and no integration tests fixtures are actually required (in test/unit/console_tool you have authorized_b2_cli and bucket fixtures that should be helpful in replacing them).
Please consider this and make your own call
There was a problem hiding this comment.
Ah, I misread your initial "it doesn't require neither shell nor API access" to mean that it does not do any API calls currently (and when I said that unit-tests are similar to integration tests, I was comparing my own tests that check b2 autocompletion vs be get-bucket autocompletion). But you're only suggesting that it shouldn't have to access the real APIs in order to check the things I want to check. Yeah, this is fair, you're right.
It's not quite as simple as just using authorized_b2_cli and bucket fixtures though because autocompleters, as they were written, create a B2Api instance independently of the one that's passed to the ConsoleTool. I'll do something about this.
* Invalidate cache when VERSION changes instead of
looking at the file hashes.
* Use platformdirs for determining cache directory.
* Move some modules to under internal _cli module.
requirements.txt
Outdated
| @@ -8,3 +8,4 @@ phx-class-registry~=4.0 | |||
| rst2ansi==0.1.5 | |||
| tabulate==0.9.0 | |||
| tqdm~=4.65.0 | |||
| platformdirs==4.0.0 | |||
There was a problem hiding this comment.
| platformdirs==4.0.0 | |
| platformdirs~=4.0.0 |
tight dependency constraints tend to be bad for anyone that wants to install this as python package. It also means we have to update it manually very often. Which is why we have started relaxing those as you can see above for new additions.
| import b2.console_tool | ||
| from b2._cli import autocomplete_cache | ||
|
|
||
| from ..console_tool.conftest import * # noqa |
There was a problem hiding this comment.
this seems entirely unnecessary; i.e. it should work exactly the same without this
There was a problem hiding this comment.
It doesn't, conftest fixtures only propagate to modules in the same directory and submodules there, don't they?
But in any case, you're right, I should extract the common fixtures to test/unit/conftest.py instead of doing this import. Done.
There was a problem hiding this comment.
Ah, or maybe the autocomplete cache tests should be moved under console_tool. The organization of tests is a bit unclear here.
|
|
||
|
|
||
| @forked | ||
| def test_complete_main_command(autocomplete_runner, tmpdir): |
There was a problem hiding this comment.
it seems tmpdir should be swapped for tmp_path everywhere
|
squash merged to upstream |
No description provided.