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

Auto-generate conf.py compatible with Py2 and Py3 #3745

Merged
merged 10 commits into from Mar 23, 2018

Conversation

humitos
Copy link
Member

@humitos humitos commented Mar 6, 2018

I found there is a mix of str compatibility types depending if we are in Py2 or Py3 when trying to auto-generate the conf.py if the project doesn't have one.

The test pushed in the first commit should pass on Py2 but should fail in Py3. This is because how open and encode behaves (I don't know the internals, though).

Ref: #3644 (comment)

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.

Great catch! I'm always hesitant to change things around encoding, so it would be good to have some solid tests here. I wasn't quite sure what this test was testing for, so that would be good to expand on.

create_index.return_value = 'README.rst'
get_config_params.return_value = {}
get_conf_py_path.side_effect = ProjectConfigurationError
self.base_sphinx.append_conf()
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is another good use case for pytest.xfail:
https://docs.pytest.org/en/latest/skipping.html#xfail-mark-test-functions-as-expected-to-fail

We can set a condition of failing on py3 for something like this.

Also, it's not clear what the test is testing for here? Is there anything to add here that would clarify that the output passed? Perhaps checking the conf file output?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it seems that it lacks of explanation.

This test probably needs the opposite of assertRaises since what I'm trying to test here is that even when a ProjectConfigurationError is raised when trying to find the conf.py (not found), the conf.py is auto-generated by RTD and it doesn't fail because of a TypeError: write() argument must be str, not SafeBytes

I'm not sure how to write it yet, but I don't think that xfail is what we want since it has to pass in both python versions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've done something like this for this case:

try:
    stuff()
except TypeError:
    pytest.fail('Reasons')

@RichardLitt RichardLitt added Needed: tests Tests are required PR: work in progress Pull request is not ready for full review labels Mar 7, 2018
@humitos humitos self-assigned this Mar 13, 2018
@humitos humitos force-pushed the humitos/conf-py-conf/auto-generated branch from a6912a4 to ff3eee4 Compare March 15, 2018 21:40
@humitos
Copy link
Member Author

humitos commented Mar 15, 2018

@agjohnson I just wrote a new test that checks the whole content of the file generated by our code against a pre-generated file. Let me know if anything...

@humitos humitos added PR: ready for review and removed Needed: tests Tests are required PR: work in progress Pull request is not ready for full review labels Mar 15, 2018
@humitos
Copy link
Member Author

humitos commented Mar 15, 2018

All tests are passing in my local instance. I don't know why it's failing in Travis :/

@humitos humitos force-pushed the humitos/conf-py-conf/auto-generated branch from 12d2457 to 468dbab Compare March 22, 2018 15:03
@humitos
Copy link
Member Author

humitos commented Mar 22, 2018

@stsewd could you run all the tests for this PR in your local instance?

I don't know why all the tests are passing locally but they fail in Travis.

@stsewd
Copy link
Member

stsewd commented Mar 22, 2018

@humitos unfortunately I see the same error as travis 😢

@humitos
Copy link
Member Author

humitos commented Mar 22, 2018

@humitos unfortunately I see the same error as travis cry

That's awesome because we could debug there :)

I will still try to make the tests fail in my local instance. I don't understand what is happening :/

@stsewd
Copy link
Member

stsewd commented Mar 22, 2018

@humitos I'll try to debug on my local instance now.

@stsewd
Copy link
Member

stsewd commented Mar 22, 2018

This is what I got from pdb

Looks like _write_config isn't doing it https://github.com/rtfd/readthedocs.org/blob/468dbabf0310515fdce6c56d5064f750bafcb23c/readthedocs/doc_builder/backends/sphinx.py#L52

So conf_file is raising the error https://github.com/rtfd/readthedocs.org/blob/468dbabf0310515fdce6c56d5064f750bafcb23c/readthedocs/projects/models.py#L542 because it doesn't find a conf.py file.

Maybe you have one created on that path?

@stsewd
Copy link
Member

stsewd commented Mar 22, 2018

Well, it is writing the file, so the lookup on the conf_file is wrong.

@stsewd
Copy link
Member

stsewd commented Mar 22, 2018

@humitos got it

The find method is searching on my local directory, not on the tmp one.

[31] > /home/stsewd/rtd/readthedocs.org/readthedocs/projects/models.py(658)find()
-> matches = []
   6 frames hidden (try 'help hidden_frames')
(Pdb++) l
653             Find files inside the project's ``doc`` path.
654
655             :param filename: Filename to search for in project checkout
656             :param version: Version instance to set version checkout path
657             """
658  ->         matches = []
659             for root, __, filenames in os.walk(self.full_doc_path(version)):
660                 for match in fnmatch.filter(filenames, filename):
661                     matches.append(os.path.join(root, match))
662             return matches
663
(Pdb++) self.full_doc_path(version)
u'/home/stsewd/rtd/readthedocs.org/user_builds/pip/checkouts/0.8.1'
(Pdb++)

@humitos
Copy link
Member Author

humitos commented Mar 22, 2018

because it doesn't find a conf.py file.

The idea of the test is to force a ProjectConfigurationError when trying to find the conf.py file so the _write_config is called.


I'm not following you.

How is that related to the error that Travis shows?

Why it doesn't fail in my local instance?

@humitos
Copy link
Member Author

humitos commented Mar 22, 2018

$ rm ./user_builds/pip/checkouts/0.8.1/conf.py

That was the solution :)

Now, I'm able to make the test fail. Thanks @stsewd!

@agjohnson agjohnson merged commit ded401e into master Mar 23, 2018
@agjohnson agjohnson deleted the humitos/conf-py-conf/auto-generated branch March 23, 2018 17:41
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.

None yet

4 participants