-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Conversation
There was a problem hiding this 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() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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')
a6912a4
to
ff3eee4
Compare
@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... |
All tests are passing in my local instance. I don't know why it's failing in Travis :/ |
12d2457
to
468dbab
Compare
@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. |
@humitos unfortunately I see the same error as travis 😢 |
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 :/ |
@humitos I'll try to debug on my local instance now. |
This is what I got from pdb Looks like So Maybe you have one created on that path? |
Well, it is writing the file, so the lookup on the |
@humitos got it The
|
The idea of the test is to force a I'm not following you. How is that related to the error that Travis shows? Why it doesn't fail in my local instance? |
That was the solution :) Now, I'm able to make the test fail. Thanks @stsewd! |
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
andencode
behaves (I don't know the internals, though).Ref: #3644 (comment)