On 5/1/2019 2:13 AM, Chris Withers wrote:

On 01/05/2019 06:12, Terry Reedy wrote:

...

Such blocks should be excluded from coverage by the default .coveragerc file.  Mine came with

exclude_lines =      # Don't complain if non-runnable code isn't run:      if 0:      if __name__ == .__main__.:      if DEBUG:

I am fairly sure these are the original lines. I added more to my personal copy to exclude some things specific to idlelib files. Idlelib files also have the same statement as the mock files. It is very handy for development.

Which .coveragerc are you referring to?

The file in the directory that contains Ned's coverage package. At least that is where it is for me.

There isn't one in the cpython repo

Since /coverage is not in the Python repo either, I would not expect it to be.

and a current release of coverage.py doesn't appear to exclude these lines for me:

https://circleci.com/gh/testing-cabal/mock/20

(line 44 in mock/tests/testsentinel.py)

I consider this a problem in the circleci coverage setup, not in the mock files. A faulty exclude-lines section leads to a faulty coverage calculation. Ned made it user-editable for a reason. -- Terry Jan Reedy

On 01/05/2019 07:46, Serhiy Storchaka wrote:

01.05.19 00:24, Chris Withers пише:

...

I have a crazy idea of getting unittest.mock up to 100% code coverage.

I noticed at the bottom of all of the test files in testmock/, there's a:

if __name__ == '__main__':      unittest.main()

...block.

How would people feel about these going away? I don't *think* they're needed now that we have unittest discover, but thought I'd ask.

These lines were added for purpose. They are needed for running tests in separate file as a script.

$ ./python Lib/unittest/test/testmock/testcallable.py -v test_attributes (__main__.TestCallable) ... ok test_create_autospec (__main__.TestCallable) ... ok test_create_autospec_instance (__main__.TestCallable) ... ok test_hierarchy (__main__.TestCallable) ... ok test_non_callable (__main__.TestCallable) ... ok test_patch_spec (__main__.TestCallable) ... ok test_patch_spec_callable_class (__main__.TestCallable) ... ok test_patch_spec_instance (__main__.TestCallable) ... ok test_patch_spec_set (__main__.TestCallable) ... ok test_patch_spec_set_instance (__main__.TestCallable) ... ok test_subclasses (__main__.TestCallable) ... ok

Right, but that's not the documented way of running individual suites in the devguide. I'm happy to remove these on the basis that there should be one and only one way of doing things like this. Chris

On 01/05/2019 13:21, Victor Stinner wrote:

Le mer. 1 mai 2019 à 03:12, Chris Withers <chris@withers.org> a écrit :

...

Right, but that's not the documented way of running individual suites in the devguide.

Maybe, but I'm using that sometimes and it's useful for some specific issues. Is it possible to run an individual test file using unittest?

Something like ./python -m unittest Lib/unittest/test/testmock/testcallable.py -v

Yep: $ ./python.exe -m unittest Lib/unittest/test/testmock/testsentinel.py ...... ---------------------------------------------------------------------- Ran 6 tests in 0.002s $ ./python.exe -m unittest -v Lib/unittest/test/testmock/testsentinel.py testBases (Lib.unittest.test.testmock.testsentinel.SentinelTest) ... ok testCopy (Lib.unittest.test.testmock.testsentinel.SentinelTest) ... ok testDEFAULT (Lib.unittest.test.testmock.testsentinel.SentinelTest) ... ok testPickle (Lib.unittest.test.testmock.testsentinel.SentinelTest) ... ok testSentinelName (Lib.unittest.test.testmock.testsentinel.SentinelTest) ... ok testSentinels (Lib.unittest.test.testmock.testsentinel.SentinelTest) ... ok ---------------------------------------------------------------------- Ran 6 tests in 0.003s OK $ ./python.exe -m unittest -v unittest.test.testmock.testsentinel testBases (unittest.test.testmock.testsentinel.SentinelTest) ... ok testCopy (unittest.test.testmock.testsentinel.SentinelTest) ... ok testDEFAULT (unittest.test.testmock.testsentinel.SentinelTest) ... ok testPickle (unittest.test.testmock.testsentinel.SentinelTest) ... ok testSentinelName (unittest.test.testmock.testsentinel.SentinelTest) ... ok testSentinels (unittest.test.testmock.testsentinel.SentinelTest) ... ok ---------------------------------------------------------------------- Ran 6 tests in 0.003s cheers, Chris

Sorry, accidentally include a comment for this in a reply to Paul: On 01/05/2019 13:39, Serhiy Storchaka wrote:

We try to support several different ways of running tests. This allows to catch some environment depended flaws in tests and serves as a kind of the test of unittest itself. Not all test files are made discoverable yet, but we move in this direction.

I'm not sure I understand how triggering via unittest or via unittest (no typo, both these __main__ blocks and python -m unittest are using the same test runner) could make a difference. And sorry, to be clear, I'm only taking about the ones in unittest/mock/testmock - Rob Collins confirmed these where never needed in a private reply:

They were never needed 😁

Removal is fine with me.

Chris

On 01/05/2019 14:52, Karthikeyan wrote:

We try to support several different ways of running tests. This allows to catch some environment depended flaws in tests and serves as a kind of the test of unittest itself. Not all test files are made discoverable yet, but we move in this direction.

Agreed. This also has helped in the past to find issues like https://bugs.python.org/issue29512#msg299045.

My read of that issue is that issue is that the only problem that was found is that an ambiguous way of running tests, which isn't included in the devguide, and maybe for good reason, caused a problem which sucked up a bunch of Victor's time. The reason Lib/test/ was on sys.path was because the tests were run with ./python Lib/test/test_bisect.py, when it shouldn't be. Had it been run with: ./ python -m test.test_bisect ...it would not have been, and bisect.py would not have needed renaming to bisect_cmd.py. Chris

On 01/05/2019 13:37, Paul Moore wrote:

I agree - removing this just to make the coverage figures look pretty seems like the wrong motivation.

Configuring coverage to understand that you want to exclude these lines from the checking would be fine, as would accepting that a coverage of slightly less than 100% is OK. Removing functionality that people use (whether or not they have other ways of getting the same results) needs a stronger justification, IMO.

It's an interesting point; I personally don't see much value in coverage of less than 100%, if you're going to look at coverage: no-one is going to focus on or potentially even notice whether coverage moved from 99% to 99% (no typo), even though a bunch of new uncovered code may have been introduced. If people are actually using these blocks, then so be it, but it feels like the people who want them to stick around are saying they're using them just on the off chance they might use them, which feels like a poor reason to keep a bunch of dead code around. I'm not sure I understand how triggering via unittest or via unittest (again, no typo, both these __main__ blocks and python -m unittest are using the same test runner) could make a difference. Chris

On 01/05/2019 14:22, Paul Moore wrote:

...

If people are actually using these blocks, then so be it, but it feels like the people who want them to stick around are saying they're using them just on the off chance they might use them, which feels like a poor reason to keep a bunch of dead code around.

If your argument was "this is dead code, and should be removed to help future maintenance", I'd have no problem with that.

Yep, that's exactly my point :-) The dev guide shows how to run individual tests without these and doesn't mention them at all. As someone coming back to core dev late last year, I was left wondering whether I *should* be using them and that created confusion for me; having only one way to do things seems like a good thing here.

My point was simply that I think that adjusting the code to make the coverage stats hit 100% feels like going at things the wrong way round.

Agreed, but my focus here is to get to 100% for mock so that it's clear that all the code is there for a reason; mock is very complicated by necessity, and having examples of why code needs to be there is what I'm aiming for most of all.

Is it really that difficult to simply tell coverage to ignore them? I thought someone had already pointed to a coveragerc file that let you do this.

It would be if the cpython repo had a coveragerc, but it does not. People maintaining their own ad-hoc coverage configs seems like a pretty bad idea.

Personally, I don't use those blocks at all, so it doesn't matter to me whether they stay or go in any practical sense.

Right, that's by gut feeling here: I don't want people encountering this mock codebase to have to wonder whether they should be running the tests using these blocks versus the way described in the dev guide, and stressing about what the differences might be, when there aren't any... Chris

On 5/1/2019 9:30 AM, Chris Withers wrote:

Agreed, but my focus here is to get to 100% for mock so that it's clear that all the code is there for a reason; mock is very complicated by necessity, and having examples of why code needs to be there is what I'm aiming for most of all.

I agree that complete 100.000% test coverage is a nice ideal, but sometimes the last percent can take hours to accomplish, if it is indeed sensibly possible. (If mock has no OS dependencies, then it may be for mock.) It is up to the individual developer to decide what is the priority to spend development time on.

...

Is it really that difficult to simply tell coverage to ignore them? I thought someone had already pointed to a coveragerc file that let you do this.

It would be if the cpython repo had a coveragerc, but it does not.

I have asked more that once what .coveragerc file is being used by CI and whether we can somehow properly customize it for CPython. I have not gotten an answer. The devguide chapter (5) on coverage is deficient in not even mentioning customization.

People maintaining their own ad-hoc coverage configs seems like a pretty bad idea.

I would prefer not having to do that. But it is better than always getting bogus numbers. At least *I* can determine the real single-file test coverage for idlelib files (on Windows), even if the public coverage reports are misleading. Unless I forget, I record it on the first line of text_xxx files.

Right, that's by gut feeling here: I don't want people encountering this mock codebase to have to wonder whether they should be running the tests using these blocks versus the way described in the dev guide,

The devguide describes the dependable but clumsy way to run tests from a command line. In my opinion, it is the worst way when one is editing a particular file. It is *much* easier to hit one key (F5 in IDLE) in an editor than to switch to a terminal window and type something like

python -m unittest idlelib.idle_test.test_configdialog

It is much better to have a SyntaxError marked in the editor than displayed in a terminal or shell. Live tracebacks in an IDE, that can jump to exception locations in an editor are better than a dead traceback in a terminal window. With IDLE there is also the issue that automated unittests cannot completely replace human action and judgment. However, displaying isolated dialogs for human interaction can be automated. The 'if main' blocks for dialog modules do so. For example, for configdialog: if __name__ == '__main__': from unittest import main main('idlelib.idle_test.test_configdialog', verbosity=2, exit=False) from idlelib.idle_test.htest import run run(ConfigDialog) One can either close the box or check the visual appearance and live behavior of the dialog. In this case, running the class is sufficient. For other modules, additional setup code is needed, which should be excluded from coverage.

and stressing about what the differences might be, when there aren't any...

Unittest users should know that it has both code and command line APIs. The devguide should mention running tests from code with main or refer to the appropriate section of the unittest doc. It should at least document the use of if __name__ == '__main__': unittest.main(verbosity=2) in test_xxx modules. -- Terry Jan Reedy

Executive summary: "There should be a tool" (sorry, I'm not volunteering any time soon) that could be added to $VCS diff (say, "git coverage-diff" or "git diff --coverage"). Chris Withers writes:

It's an interesting point; I personally don't see much value in coverage of less than 100%, if you're going to look at coverage: no-one is going to focus on or potentially even notice whether coverage moved from 99% to 99% (no typo), even though a bunch of new uncovered code may have been introduced.

I agree with the point you're making (that the difference between 100% and 99% is a very significant indicator that "something needs to be done here, it's obvious what, and [depending on project process] it's obvious who, too"), but it's also true that 99% is better than 98% and definitely better than 90% or less. Your point that it matters *which* 1% is more important, I think (see "Executive summary").

If people are actually using these blocks, then so be it, but it feels like the people who want them to stick around are saying they're using them just on the off chance they might use them, which feels like a poor reason to keep a bunch of dead code around.

At least one person says he uses it, although I don't know how that fits with Robert's statement that "it was never needed". Steve

I thought https://www.python.org/dev/peps/pep-0582/ had something about no longer add '.' to sys.path, but looks like I was mistaken.

It was discussed [0] in the past with respect to security since Perl did a similar change and PEP 432 is also mentioned in the thread. [1] [0] https://mail.python.org/pipermail/python-ideas/2017-June/045842.html [1] https://mail.python.org/pipermail/python-ideas/2017-June/045849.html -- Regards, Karthikeyan S