Refactor src/_pytest/config/__init__.py to use the warnings module instead of stderr for warnings

[gnikonorov]

Closes #7295

Refactors src/_pytest/config/__init__.py to use the warnings module instead of writing out warnings to stderr

[gnikonorov]

Hey @nicoddemus @RonnyPfannschmidt - any idea why codecov says I only hit 66% of the diff? I ( think I ) have adequate testing

[RonnyPfannschmidt]

based on the coverage diff view i would guess that the assertion disabled case was not hit by coverage, which it should be if i remember correct

as such i believe a investigation is needed into that,

[RonnyPfannschmidt]

unfortunately i wont be able to do any deep work on pytest until the next weekend

[gnikonorov]

Do we want to allow this to merge before that? Or should we hold off until we check codecov

[RonnyPfannschmidt]

I'll defer to @nicoddemus on that one

[gnikonorov]

This definitely looks like an issue with codecov, and not the PR.

I updated this PR with 33de350 to parametrize some of the tests, and it looks like that dropped coverage from 66% to 60% even though no coverage was added or taken away.

I suggest we open an issue to track this and treat it separately @nicoddemus and @RonnyPfannschmidt

[gnikonorov]

@nicoddemus are you opposed to merging this in and treating the codecov as a separate issue?

[gnikonorov]

Gentle ping @nicoddemus

[bluetech]

Nice cleanup! I left a few minor comments/questions.

As a follow up I think it would be good to move _issue_warning_captured from _pytest.warning to some more suitable place, probably _pytest.config, since it's obviously misplaced now - every single caller of it imports it lazily due to import cycles...

[bluetech]

I'm always not 100% which value it should be, so just making sure, did you check the stacklevel=2 is the best value here? I think it would be best to point at the caller of _warn_or_fail_if_strict.

[gnikonorov]

the caller of _validate_keys is _preparse, which already had stacklevel=2, so we'd want 2 here as well right @bluetech ?

[bluetech]

Same question here re. stacklevel.

[gnikonorov]

same reply as to your original question. That would mean we want stacklevel 2 here as well right?

[bluetech]

BTW when I checked it out there was a minor conflict with master around the create_terminal_writer function, so a rebase would be good.

[nicoddemus]

As a follow up I think it would be good to move _issue_warning_captured from _pytest.warning to some more suitable place, probably _pytest.config, since it's obviously misplaced now - every single caller of it imports it lazily due to import cycles...

Definitely agree, every caller also has access to config, so it makes perfect sense.

[nicoddemus]

Looks great @gnikonorov thanks!

Please take a look at the comments made by @bluetech, I think they are worth doing/checking.

We should follow up with making _issue_warning_captured a method of Config after this gets merged.

[gnikonorov]

BTW when I checked it out there was a minor conflict with master around the create_terminal_writer function, so a rebase would be good.

@bluetech - I looked at the diff and I don't see any changes/conflicts, and just merged in upstream. What do you see is inconsistent?

[bluetech]

I looked at the diff and I don't see any changes/conflicts, and just merged in upstream. What do you see is inconsistent?

I guess it was just local, github didn't complain as well. Anyway it's good now, thanks!

(BTW, I personally prefer rebasing PRs instead of merging master into them, I find that a little cleaner, but whatever you prefer is OK).

the caller of _validate_keys is _preparse, which already had stacklevel=2, so we'd want 2 here as well right

I'm not sure I understand, so let me explain more what I mean. The stacklevel determines which line the warning message will appear to come from for the user. For example for the strict config warning, with stacklevel=2 the warning looks like this:

src/_pytest/config/__init__.py:1188
  /home/ran/src/pytest/src/_pytest/config/__init__.py:1188: PytestConfigWarning: Unknown config ini key: bla
  
    _issue_warning_captured(

the question is whatever _issue_warning_captured is the best place to point to. For reference, here it is with different values.

0:

src/_pytest/warnings.py:205
  /home/ran/src/pytest/src/_pytest/warnings.py:205: PytestConfigWarning: Unknown config ini key: bla
  
    warnings.warn(warning, stacklevel=0)

1:

src/_pytest/warnings.py:205
  /home/ran/src/pytest/src/_pytest/warnings.py:205: PytestConfigWarning: Unknown config ini key: bla
  
    warnings.warn(warning, stacklevel=1)

2: See above.

3:

src/_pytest/config/__init__.py:1148
  /home/ran/src/pytest/src/_pytest/config/__init__.py:1148: PytestConfigWarning: Unknown config ini key: bla
  
    self._warn_or_fail_if_strict("Unknown config ini key: {}\n".format(key))

4:

src/_pytest/config/__init__.py:1125
  /home/ran/src/pytest/src/_pytest/config/__init__.py:1125: PytestConfigWarning: Unknown config ini key: bla
  
    self._validate_keys()

5:

src/_pytest/config/__init__.py:1204
  /home/ran/src/pytest/src/_pytest/config/__init__.py:1204: PytestConfigWarning: Unknown config ini key: bla
  
    self._preparse(args, addopts=addopts)

etc... In this case IMO the best one is stacklevel=3.

[gnikonorov]

Thanks for the explanation @bluetech ! I agree, and changed the stack level to 3 for my changes

I also made a note in #7426 to re-evaluate the stack levels we currently use. We use 2 in this file. perhaps here ( and elsewhere ) we can change the stack level to be more appropriate

[bluetech]

Merged myself to override the patch coverage issue