Remove pytest _describe #4429
Remove pytest _describe #4429
Conversation
mashrikt
force-pushed
the
pytest_describe_remove
branch
2 times, most recently
from
d84f383 to
e0da5e3
Compare
| @@ -222,15 +222,15 @@ def test_python_pip_install_default(): | |||
| assert build.pip_install is False | |||
|
|
|||
|
|
|||
| def describe_validate_python_extra_requirements(): | |||
| class ValidatePythonExtraRequirements: | |||
There was a problem hiding this comment.
We need to follow the convention of naming the class starting with Test like TestValidatePythonExtraRequirements and inheritance from object to keep the code python2 compatible.
|
|
||
| def it_defaults_to_list(): | ||
| def it_defaults_to_list(self): |
There was a problem hiding this comment.
Also in the methods, we need to start the name with test_
There was a problem hiding this comment.
Thanks for looking at this, we need to follow some conventions when naming the tests classes to pytest be able to discover them.
I think we may want to wait at least https://github.com/rtfd/readthedocs.org/pull/4355/files is merged, that way we avoid a lot of conflicts later.
|
@stsewd I have made made the changes. |
mashrikt
force-pushed
the
pytest_describe_remove
branch
from
257f906 to
1f5b577
Compare
mashrikt
force-pushed
the
pytest_describe_remove
branch
from
1f5b577 to
c07316b
Compare
|
https://github.com/rtfd/readthedocs.org/pull/4355/files is merged. |
|
@mashrikt I have restarted the test! |
|
@mashrikt could you rebase/update the branch? |
mashrikt
force-pushed
the
pytest_describe_remove
branch
from
c07316b to
7937c4b
Compare
|
@stsewd Done! |
|
|
||
| def it_defaults_to_list(): | ||
| def test_it_defaults_to_list(self): |
There was a problem hiding this comment.
not sure if we need the it in the tests names anymore, what do you think?
There was a problem hiding this comment.
I agree. I was thinking the same.
Should the naming convention be:
i) test_default_to_list or
ii) test_defaults_to_list?
There was a problem hiding this comment.
I'm not an english expert, but I guess 2 makes sense.
|
|
||
| def it_defaults_to_list(): | ||
| def test_it_defaults_to_list(self): |
There was a problem hiding this comment.
I agree. I was thinking the same.
Should the naming convention be:
i) test_default_to_list or
ii) test_defaults_to_list?
|
@stsewd want to give this another look and see if we can get it approved and merged? |
|
As a note, I'm happy to merge it with the current naming, so if everything looks good, we should 👍 |
|
@ericholscher yeah, we are good to merge this, codecov bot didn't got triggered here :/ |
Codecov Report
@@ Coverage Diff @@
## master #4429 +/- ##
=======================================
Coverage 76.21% 76.21%
=======================================
Files 158 158
Lines 10019 10019
Branches 1265 1265
=======================================
Hits 7636 7636
Misses 2039 2039
Partials 344 344 |
This refers to #4270. Removes the pytest_describe blocks with class.
I am told that it is blocked until https://github.com/orgs/rtfd/projects/2 is resolved.