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

Remove pytest _describe #4429

Merged
merged 5 commits into from Oct 31, 2018
Merged

Conversation

@mashrikt
Copy link
Contributor

@mashrikt mashrikt commented Jul 25, 2018

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.

@mashrikt mashrikt force-pushed the pytest_describe_remove branch 2 times, most recently from d84f383 to e0da5e3 Jul 25, 2018
@@ -222,15 +222,15 @@ def test_python_pip_install_default():
assert build.pip_install is False


def describe_validate_python_extra_requirements():
class ValidatePythonExtraRequirements:
Copy link
Member

@stsewd stsewd Jul 26, 2018

Choose a reason for hiding this comment

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

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.

Loading


def it_defaults_to_list():
def it_defaults_to_list(self):
Copy link
Member

@stsewd stsewd Jul 26, 2018

Choose a reason for hiding this comment

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

Also in the methods, we need to start the name with test_

Loading

Copy link
Contributor Author

@mashrikt mashrikt Jul 26, 2018

Choose a reason for hiding this comment

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

Right! My bad.

Loading

Copy link
Member

@stsewd stsewd left a 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.

Loading

@mashrikt
Copy link
Contributor Author

@mashrikt mashrikt commented Jul 26, 2018

@stsewd I have made made the changes.
I understand your point. Happy to rebase and fix when the other PR is merged.

Loading

@mashrikt mashrikt force-pushed the pytest_describe_remove branch from 257f906 to 1f5b577 Jul 26, 2018
@mashrikt mashrikt force-pushed the pytest_describe_remove branch from 1f5b577 to c07316b Aug 2, 2018
@mashrikt
Copy link
Contributor Author

@mashrikt mashrikt commented Aug 2, 2018

https://github.com/rtfd/readthedocs.org/pull/4355/files is merged.
So I have rebased this PR. But checks have failed.
@stsewd

Loading

@safwanrahman
Copy link
Member

@safwanrahman safwanrahman commented Aug 2, 2018

@mashrikt I have restarted the test!
Update: Test passed!

Loading

@agjohnson agjohnson added this to the Refactoring milestone Aug 27, 2018
@stsewd
Copy link
Member

@stsewd stsewd commented Aug 30, 2018

@mashrikt could you rebase/update the branch?

Loading

@mashrikt mashrikt force-pushed the pytest_describe_remove branch from c07316b to 7937c4b Aug 30, 2018
@mashrikt
Copy link
Contributor Author

@mashrikt mashrikt commented Aug 30, 2018

@stsewd Done!

Loading


def it_defaults_to_list():
def test_it_defaults_to_list(self):
Copy link
Member

@stsewd stsewd Aug 30, 2018

Choose a reason for hiding this comment

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

not sure if we need the it in the tests names anymore, what do you think?

Loading

Copy link
Contributor Author

@mashrikt mashrikt Sep 27, 2018

Choose a reason for hiding this comment

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

I agree. I was thinking the same.
Should the naming convention be:
i) test_default_to_list or
ii) test_defaults_to_list?

Loading

Copy link
Member

@stsewd stsewd Sep 27, 2018

Choose a reason for hiding this comment

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

I'm not an english expert, but I guess 2 makes sense.

Loading

Copy link
Member

@ericholscher ericholscher Oct 31, 2018

Choose a reason for hiding this comment

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

Yea, 2 is 👍

Loading

Copy link
Contributor Author

@mashrikt mashrikt left a comment

Should the naming convention be:
i) test_default_to_list or
ii) test_defaults_to_list?

Loading


def it_defaults_to_list():
def test_it_defaults_to_list(self):
Copy link
Contributor Author

@mashrikt mashrikt Sep 27, 2018

Choose a reason for hiding this comment

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

I agree. I was thinking the same.
Should the naming convention be:
i) test_default_to_list or
ii) test_defaults_to_list?

Loading

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Oct 31, 2018

@stsewd want to give this another look and see if we can get it approved and merged?

Loading

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Oct 31, 2018

As a note, I'm happy to merge it with the current naming, so if everything looks good, we should 👍

Loading

@stsewd
Copy link
Member

@stsewd stsewd commented Oct 31, 2018

@ericholscher yeah, we are good to merge this, codecov bot didn't got triggered here :/

Loading

@ericholscher ericholscher merged commit 4984776 into readthedocs:master Oct 31, 2018
1 check passed
Loading
@codecov
Copy link

@codecov codecov bot commented Oct 31, 2018

Codecov Report

Merging #4429 into master will not change coverage.
The diff coverage is n/a.

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

Loading

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

5 participants