Refs #32552 -- Added DiscoverRunner.log() to allow customization. by abbasidaniyal · Pull Request #14150 · django/django

abbasidaniyal · 2021-03-19T09:23:58Z

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 · 2021-03-19T09:27:33Z

The tests for DiscoverRunner

django/tests/test_runner/test_discover_runner.py

Lines 336 to 337 in 3704481

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

and

django/tests/test_runner/test_discover_runner.py

Lines 252 to 262 in 3704481

    
           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?

cjerdonek

Thanks. One comment..

django/test/runner.py

cjerdonek · 2021-03-19T10:54:41Z

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.

cjerdonek · 2021-03-21T01:14:04Z

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 · 2021-03-26T21:00:38Z

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

cjerdonek

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

carltongibson

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 · 2021-04-08T14:12:00Z

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.

cjerdonek

Some comments.

django/test/runner.py

docs/topics/testing/advanced.txt

tests/test_runner/test_discover_runner.py

django/test/runner.py

cjerdonek

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

django/test/runner.py

cjerdonek · 2021-04-18T22:45:23Z

docs/topics/testing/advanced.txt

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

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

cjerdonek · 2021-04-18T22:45:44Z

tests/test_runner/test_discover_runner.py

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

tests/test_runner/test_discover_runner.py

django/test/runner.py

smithdc1

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

tests/test_runner/test_discover_runner.py

docs/topics/testing/advanced.txt

Thanks Carlton Gibson, Chris Jerdonek, and David Smith for reviews.

felixxm · 2021-06-01T11:32:48Z

Thanks y'all 🚀

I rebased and pushed small edits.

Stale

cjerdonek reviewed Mar 19, 2021

View reviewed changes

django/test/runner.py Outdated Show resolved Hide resolved

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

abbasidaniyal force-pushed the ticket_32552 branch from 4ad06b3 to 330de72 Compare March 26, 2021 20:51

cjerdonek suggested changes Mar 27, 2021

View reviewed changes

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

abbasidaniyal force-pushed the ticket_32552 branch from 330de72 to 8b221a2 Compare April 8, 2021 00:12

carltongibson previously requested changes Apr 8, 2021

View reviewed changes

abbasidaniyal requested a review from carltongibson April 8, 2021 14:10

cjerdonek reviewed Apr 17, 2021

View reviewed changes

abbasidaniyal force-pushed the ticket_32552 branch 2 times, most recently from 737caf9 to cf9ecc1 Compare April 18, 2021 21:13

cjerdonek reviewed Apr 18, 2021

View reviewed changes

abbasidaniyal force-pushed the ticket_32552 branch from cf9ecc1 to a190cc7 Compare April 19, 2021 08:53

cjerdonek approved these changes Apr 19, 2021

View reviewed changes

abbasidaniyal requested a review from smithdc1 April 19, 2021 09:07

smithdc1 reviewed Apr 19, 2021

View reviewed changes

django/test/runner.py Outdated 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 removed the request for review from carltongibson June 1, 2021 10:10

felixxm self-assigned this Jun 1, 2021

felixxm changed the title ~~Refs #32552 -- Added a DiscoverRunner.log() method~~ Refs #32552 -- Added DiscoverRunner.log() to allow customization. Jun 1, 2021

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

a0410ff

Thanks Carlton Gibson, Chris Jerdonek, and David Smith for reviews.

felixxm force-pushed the ticket_32552 branch from a190cc7 to a0410ff Compare June 1, 2021 11:32

felixxm merged commit a0410ff into django:main Jun 1, 2021

jacobtylerwalls mentioned this pull request Aug 7, 2021

Fixed #29026 -- Added --scriptable option to makemigrations. #14751

Merged

Uh oh!

Conversation

abbasidaniyal commented Mar 19, 2021 • edited by carltongibson Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abbasidaniyal commented Mar 19, 2021

Uh oh!

cjerdonek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cjerdonek commented Mar 19, 2021

Uh oh!

cjerdonek commented Mar 21, 2021

Uh oh!

abbasidaniyal commented Mar 26, 2021

Uh oh!

cjerdonek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carltongibson left a comment

Choose a reason for hiding this comment

Uh oh!

abbasidaniyal commented Apr 8, 2021

Uh oh!

cjerdonek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cjerdonek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cjerdonek Apr 18, 2021

Choose a reason for hiding this comment

Uh oh!

smithdc1 Apr 19, 2021

Choose a reason for hiding this comment

Uh oh!

cjerdonek Apr 18, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smithdc1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

felixxm commented Jun 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

abbasidaniyal commented Mar 19, 2021 •

edited by carltongibson

Loading