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

[RDY] Fix requirements file lookup #3800

Merged
merged 13 commits into from Apr 17, 2018

Conversation

Projects
None yet
3 participants
@stsewd
Member

stsewd commented Mar 14, 2018

Fix #2812

I didn't find any place that match the tests, so I added a new class.

@stsewd stsewd changed the title from [WIP] Fix requirements file lookup to Fix requirements file lookup Mar 14, 2018

@stsewd stsewd changed the title from Fix requirements file lookup to [RDY] Fix requirements file lookup Mar 14, 2018

@stsewd

This comment has been minimized.

Member

stsewd commented Mar 14, 2018

Added some additional tests for Conda environment :)

@stsewd

This comment has been minimized.

Member

stsewd commented Mar 14, 2018

Before: Coverage for doc_builder/python_environments.py : 44%
After: Coverage for doc_builder/python_environments.py : 73%

@humitos

I liked those tests!

I didn't get what's the change in the logic what fixes the lookup. I left a question for that.

Also, I'd like you to refactor a little bit the test cases before merging. I left some comments with suggestions on how to improve them.

Besides that, I think there are no major issues to change/modify.

break
paths = [docs_dir, '']
req_files = ['pip_requirements.txt', 'requirements.txt']
for path, req_file in itertools.product(paths, req_files):

This comment has been minimized.

@humitos

humitos Mar 23, 2018

Member

I don't see any logic change here, right?

We just had 2 nested for and now we use itertools.products that produces the same result in the end. Am I right?

This comment has been minimized.

@humitos

humitos Mar 23, 2018

Member
>>> import itertools
>>> d = ['a', 'b']
>>> r = [1, 2]
>>> iter
iter(      itertools  
>>> list(itertools.product(d, r))
[('a', 1), ('a', 2), ('b', 1), ('b', 2)]
>>> for _d in d:
...     for _r in r:
...         print(_d, _r)
... 
a 1
a 2
b 1
b 2
>>> 

This comment has been minimized.

@stsewd

stsewd Mar 23, 2018

Member

It is the same result, but the break now exists the loop #2812 (comment)

This comment has been minimized.

@humitos

humitos Mar 23, 2018

Member

Oh boy! Great catch! :)

build_env=self.build_env_mock,
)
python_env.install_core_requirements()
requirements = [

This comment has been minimized.

@humitos

humitos Mar 23, 2018

Member

Use the same list of reqs (common) for both project (created in setUp) and append the ones that are specific for documentation type in each test.

This will be easier to follow and to change when we upgrade the reqs also.

'requirements_file'
]
paths[docs_requirements] = True

This comment has been minimized.

@humitos

humitos Mar 23, 2018

Member

Can you add a simple comment in each of the cases saying what's the scenario and what we expect?

)
def test_install_user_requirements(self):
self.build_env_mock.project = self.project_sphinx

This comment has been minimized.

@humitos

humitos Mar 23, 2018

Member

Can you add a docstring here explaining the test case in general?

paths[root_requirements] = False
with fake_paths_lookup(paths):
python_env.install_user_requirements()
args[-1] = '-r{}'.format(docs_requirements)

This comment has been minimized.

@humitos

humitos Mar 23, 2018

Member

I don't like this modification of the list, but I don't have a cleaner way of doing it :/

This comment has been minimized.

@stsewd

stsewd Mar 23, 2018

Member

I didn't like it either, but it was the only way I thought to reuse the previous list p:

build_env=self.build_env_mock,
)
python_env.install_core_requirements()
conda_requirements = [

This comment has been minimized.

@humitos

humitos Mar 23, 2018

Member

Same here, try to use a shared list.

'python',
mock.ANY, # pip path
'install',
'-U',

This comment has been minimized.

@humitos

humitos Mar 23, 2018

Member

You can also use a shared args_pip for all these tests. You will need to change the -U by --upgrade but I think that's OK and it's better to use long attributes instead of short. So, 👍

This comment has been minimized.

@stsewd

stsewd Mar 23, 2018

Member

There is two types of the arg_pip, on conda the --use-wheel isn't used. Is that on purpose? or perhaps we can unify both :)?

# No requirements file
# no requirements should be installed
self.build_env_mock.run.reset_mock()

This comment has been minimized.

@stsewd

stsewd Mar 23, 2018

Member

With the last update of the mock lib, the assert_not_called() was counting the previous calls too, so I have to reset it here.

@stsewd

This comment has been minimized.

Member

stsewd commented Mar 23, 2018

@humitos I updated the tests, and now there is a shorter file, I think this can be even shorter if this is possible #3800 (comment)

@humitos

I really like these changes!

From my point of view, this is ready to merge.

@humitos humitos merged commit bf97bc5 into rtfd:master Apr 17, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stsewd stsewd deleted the stsewd:fix-requirements-file-lookup branch Apr 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment