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

Refs #32552 -- Added DiscoverRunner.log() to allow customization. #14150

Merged
merged 1 commit into from Jun 1, 2021

Conversation

abbasidaniyal
Copy link
Contributor

@abbasidaniyal abbasidaniyal commented Mar 19, 2021

ticket-32552

Add a method log(self, message, level) to DiscoverRunner class which prints the message by comparing the level with self.verbosity.
Replace the use of print with self.log

@abbasidaniyal
Copy link
Contributor Author

The tests for DiscoverRunner

class DiscoverRunnerGetDatabasesTests(SimpleTestCase):
runner = DiscoverRunner(verbosity=2)

and
def test_included_tags_displayed(self):
runner = DiscoverRunner(tags=['foo', 'bar'], verbosity=2)
with captured_stdout() as stdout:
runner.build_suite(['test_runner_apps.tagged.tests'])
self.assertIn('Including test tag(s): bar, foo.\n', stdout.getvalue())
def test_excluded_tags_displayed(self):
runner = DiscoverRunner(exclude_tags=['foo', 'bar'], verbosity=3)
with captured_stdout() as stdout:
runner.build_suite(['test_runner_apps.tagged.tests'])
self.assertIn('Excluding test tag(s): bar, foo.\n', stdout.getvalue())

only check for verbosity >= 2. I believe we should add tests for verbosity < 2 as well. Thoughts?

Copy link
Contributor

@cjerdonek cjerdonek left a comment

Choose a reason for hiding this comment

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

Thanks. One comment..

django/test/runner.py Outdated Show resolved Hide resolved
@cjerdonek
Copy link
Contributor

Also, the title of the PR should be something like "Added a DiscoverRunner.log() method" since you're not actually changing DiscoverRunner to use a logger here.

@abbasidaniyal abbasidaniyal changed the title Refs #32552 -- Change DiscoverRunner to use a logger instead of print Refs #32552 -- Added a DiscoverRunner.log() method Mar 20, 2021
@cjerdonek
Copy link
Contributor

I believe we should add tests for verbosity < 2 as well. Thoughts?

I would add a single new test called test_log(). It can use subTest() to test the following matrix of 12 possibilities: verbosity (0, 1, 2, 3) times level (DEBUG, INFO, WARN).

@abbasidaniyal
Copy link
Contributor Author

Just made few changes. Let me know if this seems better. Working on the tests!

Copy link
Contributor

@cjerdonek cjerdonek left a comment

Choose a reason for hiding this comment

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

Just made few changes. Let me know if this seems better.

It looks like you didn't finish addressing my previous comments: #14150 (comment)

I've repeated them inline with some additional explanation and added a couple other comments.

django/test/runner.py Outdated Show resolved Hide resolved
django/test/runner.py Outdated Show resolved Hide resolved
django/test/runner.py Outdated Show resolved Hide resolved
django/test/runner.py Outdated Show resolved Hide resolved
Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

Hi @abbasidaniyal — Thanks for updating this.

  • Please look at the existing test failures.
  • Also see Chris' comment about adding a new test_log(), with the appropriate matrix of combinations. (See subTest Python docs and examples in the test suite for usage.)

Once those are in place and addition to the DiscoverRunner docs and a release note in releases/4.0.txt would be appropriate.

@abbasidaniyal
Copy link
Contributor Author

Hi @abbasidaniyal — Thanks for updating this.

  • Please look at the existing test failures.
  • Also see Chris' comment about adding a new test_log(), with the appropriate matrix of combinations. (See subTest Python docs and examples in the test suite for usage.)

Once those are in place and addition to the DiscoverRunner docs and a release note in releases/4.0.txt would be appropriate.

Just made these changes. Not sure if the docs and the release notes are under the appropriate heading, let me know if I need to make amends.

Copy link
Contributor

@cjerdonek cjerdonek left a comment

Choose a reason for hiding this comment

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

Some comments.

django/test/runner.py Outdated Show resolved Hide resolved
docs/topics/testing/advanced.txt Outdated Show resolved Hide resolved
tests/test_runner/test_discover_runner.py Outdated Show resolved Hide resolved
tests/test_runner/test_discover_runner.py Show resolved Hide resolved
django/test/runner.py Outdated Show resolved Hide resolved
@abbasidaniyal abbasidaniyal force-pushed the ticket_32552 branch 2 times, most recently from 737caf9 to cf9ecc1 Compare April 18, 2021 21:13
Copy link
Contributor

@cjerdonek cjerdonek left a comment

Choose a reason for hiding this comment

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

Okay, last round of comments (I think?). They're more minor than before. Thanks again for keeping at it, @abbasidaniyal.

django/test/runner.py Outdated Show resolved Hide resolved
django/test/runner.py Outdated Show resolved Hide resolved
django/test/runner.py Show resolved Hide resolved
django/test/runner.py Outdated Show resolved Hide resolved
django/test/runner.py Show resolved Hide resolved
.. method:: DiscoverRunner.log(msg, level=None)

Logs a message at the given `logging level <https://docs.python.org/3/library/logging.html#levels>`_
respecting the current ``verbosity``. For example, INFO will be logged if the verbosity is at least 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Backticks are needed around INFO, 1, DEBUG, and 2.

Copy link
Member

Choose a reason for hiding this comment

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

Please also add a .. versionadded 4.0 directive. With a shirt note saying this method was added.

https://docs.djangoproject.com/en/dev/internals/contributing/writing-documentation/#documenting-new-features

(3, logging.INFO, True),
(3, logging.WARNING, True)
]

Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove the empty line above and the two empty lines below.

tests/test_runner/test_discover_runner.py Outdated Show resolved Hide resolved
tests/test_runner/test_discover_runner.py Outdated Show resolved Hide resolved
django/test/runner.py Outdated Show resolved Hide resolved
Copy link
Member

@smithdc1 smithdc1 left a comment

Choose a reason for hiding this comment

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

Your prompt for a review made me look at this again. 😄

A few small questions/comments, I suspect most can be ignored.

django/test/runner.py Show resolved Hide resolved
tests/test_runner/test_discover_runner.py Outdated Show resolved Hide resolved
docs/topics/testing/advanced.txt Outdated Show resolved Hide resolved
@felixxm felixxm removed the request for review from carltongibson June 1, 2021 10:10
@felixxm felixxm self-assigned this Jun 1, 2021
@felixxm felixxm changed the title Refs #32552 -- Added a DiscoverRunner.log() method Refs #32552 -- Added DiscoverRunner.log() to allow customization. Jun 1, 2021
Thanks Carlton Gibson, Chris Jerdonek, and David Smith for reviews.
@felixxm
Copy link
Member

felixxm commented Jun 1, 2021

Thanks y'all 🚀

I rebased and pushed small edits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants