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

More robusts tests for requirements_file and conda_file #4394

Merged

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Jul 17, 2018

Previous tests were mocking the load_config function, now we are testing it for real

Related to #4379

@stsewd stsewd requested a review from a team July 17, 2018 20:16
})
@mock.patch('readthedocs.projects.models.Project.checkout_path')
def test_conda_with_cofig(self, checkout_path):
base_path = tempfile.mkdtemp()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't able to reuse the apply_fs function (only works with pytest) or fake_paths as the check is very strict.

@@ -40,14 +44,30 @@ def inner(path=None, env_config=None):
return inner


@mock.patch('readthedocs.doc_builder.config.load_config')
Copy link
Member Author

@stsewd stsewd Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to remove the class level mock and only used it in some methods

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

"""
Creates a readthedocs configuration file with name
``file_name`` in ``base_path``. If ``base_path`` is not given
a temporal directory is created.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: not to change now, but just to consider that PEP257 (https://www.python.org/dev/peps/pep-0257/#multi-line-docstrings) says that we need one summary line, a blank line, and a more elaborate description.

I know that you love following PEPs ;)

NOTE: this is checked automatically by one of our linters and it will fail in that case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha, yeah


full_requirements_file = path.join(base_path, requirements_file)
with open(full_requirements_file, 'w') as f:
f.write('pip')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to create this file at all for the test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the validation is very strict, and it checks that the file exists, I wasn't able to mock this one. But I'm using pytest for our new tests, and we don't need to use this hacks anymore

@stsewd stsewd force-pushed the better-tests-for-config-integration branch from 2703a5b to 773cbff Compare August 29, 2018 18:08
@stsewd
Copy link
Member Author

stsewd commented Aug 29, 2018

Rebased and updated

@stsewd stsewd force-pushed the better-tests-for-config-integration branch from 773cbff to d8c173c Compare September 12, 2018 18:42
@stsewd stsewd force-pushed the better-tests-for-config-integration branch from d8c173c to 0774e23 Compare September 26, 2018 19:05
Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@agjohnson agjohnson merged commit 84a9b6c into readthedocs:master Sep 27, 2018
@stsewd stsewd deleted the better-tests-for-config-integration branch September 27, 2018 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants