Skip to content
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

@stsewd
Copy link
Member

@stsewd 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 [WIP] Fix requirements file lookup Fix requirements file lookup Mar 14, 2018
@stsewd stsewd changed the title Fix requirements file lookup [RDY] Fix requirements file lookup Mar 14, 2018
@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 14, 2018

Added some additional tests for Conda environment :)

@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 14, 2018

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

Copy link
Member

@humitos humitos left a comment

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):
Copy link
Member

@humitos humitos Mar 23, 2018

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?

Copy link
Member

@humitos humitos Mar 23, 2018

>>> 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
>>> 

Copy link
Member Author

@stsewd stsewd Mar 23, 2018

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

Copy link
Member

@humitos humitos Mar 23, 2018

Oh boy! Great catch! :)

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

@humitos humitos Mar 23, 2018

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
Copy link
Member

@humitos humitos Mar 23, 2018

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
Copy link
Member

@humitos humitos Mar 23, 2018

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)
Copy link
Member

@humitos humitos Mar 23, 2018

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

Copy link
Member Author

@stsewd stsewd Mar 23, 2018

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 = [
Copy link
Member

@humitos humitos Mar 23, 2018

Same here, try to use a shared list.

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

@humitos humitos Mar 23, 2018

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, 👍

Copy link
Member Author

@stsewd stsewd Mar 23, 2018

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()
Copy link
Member Author

@stsewd stsewd Mar 23, 2018

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
Copy link
Member Author

@stsewd 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)

Copy link
Member

@humitos humitos left a comment

I really like these changes!

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

@humitos humitos merged commit bf97bc5 into readthedocs:master Apr 17, 2018
1 check passed
@stsewd stsewd deleted the 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
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants