#7336 make --log-file relative to inifile if it exists by symonk · Pull Request #7350 · pytest-dev/pytest

symonk · 2020-06-11T12:56:22Z

As per #7336, I think the correct behaviour is to keep the --log-file relative to the inifile? From what I gather the inifile can be None; so this PR makes the --log-file relative to that directory in the event that it exists; not sure how to proceed if it is None in a good manner. (assume the current behaviour of putting the log file in the execution directory?)

Close #7336

gnikonorov · 2020-06-11T19:40:53Z

While this is technically beyond the scope of the linked issue can we allow for the user to provide an absolute path for a log file, in which case we do not make the log file relative to anything?

bluetech

Hi @symonk, some comments.

Besides __init__, the function set_log_path contains some logic relating to this, and there it is using rootdir. They should probably be reconciled in some way.

bluetech · 2020-06-11T19:49:08Z

src/_pytest/logging.py

        log_file = get_option_ini(config, "log_file") or os.devnull
+
+        # Keep the log file relative to the inidir file (if it exists) (#7336)
+        inidir = getattr(getattr(config, "inifile", None), "dirname", None)


At this point config.inifile should always be defined (unless I'm mistaken), so getattr(config, "inifile", None) can be replaced with just config.inifile.

inifile can be None, but instead of using getattr on it, I think it is better do use an explicit is None check. getattr() is using a dynamic feature of Python which is not analyzed and type checked, so I for one prefer to avoid it unless the dynamicism is really needed (e.g. when the name is not a constant).

Finally, I think currently there may be some cases where inifile may be an str rather than a py.path.local, but that is more of a bug (I'm just working on that as a follow up for the recent TOML PR).

Hmm I seem to be mistaken, I thought i was getting a type:str in some cases; and no 'dirname' attr, but it seems if config.inifile is not None, it is always path.local (as far as our current test suite goes)

mypy hates:

src\_pytest\logging.py:533: error: "str" has no attribute "dirname" [attr-defined] Found 1 error in 1 file (checked 2 source files)

I assume some type hinting on the config obj - will check

bluetech · 2020-06-11T20:00:07Z

While this is technically beyond the scope of the linked issue can we allow for the user to provide an absolute path for a log file, in which case we do not make the log file relative to anything?

You usually get this "for free" from the way path-joining works:

>>> import os.path
>>> os.path.join('/the/log/dir', '/an/absolute/path')
'/an/absolute/path'

nicoddemus

Thanks @symonk,

Looks good, just left a few comments. 👍

nicoddemus · 2020-06-11T19:58:40Z

testing/logging/test_reporting.py

+def test_log_file_is_in_pytest_ini_rootdir(testdir):
+    # as per https://docs.pytest.org/en/5.4.3/reference.html log_file
+    # the file should be relative to the pytest inifile
+    testdir.makefile(


No need to change, but you can also use makeinifile. 😉

ah i got confused as it made a 'tox' file :)

Probably for historical reasons.

nicoddemus · 2020-06-11T20:00:27Z

testing/logging/test_reporting.py

    result.assert_outcomes(passed=1)
+
+
+def test_log_file_is_in_pytest_ini_rootdir(testdir):


Please add docstrings to each test referencing #7350, so it is easier to find the issue that lead to the creation of the test later. 👍

thanks, i assume a typo of #7350/#7336 - will update

oops yes 😁

nicoddemus · 2020-06-11T20:02:16Z

src/_pytest/logging.py

        log_file = get_option_ini(config, "log_file") or os.devnull
+
+        # Keep the log file relative to the inidir file (if it exists) (#7336)
+        inidir = getattr(getattr(config, "inifile", None), "dirname", None)


I realize this is a matter of opinion, but this seems a bit contrived, I would instead use this:

if log_file != os.devnull and config.inifile: log_file = os.path.join(config.inifile.dirname, log_file)

(No need for the getattr chain)

Thanks, any advice on fixing:

src\_pytest\logging.py:533: error: "str" has no attribute "dirname" [attr-defined] Found 1 error in 1 file (checked 2 source files)

As a result?

I see:

def determine_setup( inifile: Optional[str], args: List[str], rootdir_cmd_arg: Optional[str] = None, config: Optional["Config"] = None, ) -> Tuple[py.path.local, Optional[str], Dict[str, Union[str, List[str]]]]:

should this be a path local?

if im not mistaken, dont we want to remove references to 'py' ?

That's weird, locate_config returns:

Tuple[ Optional[py.path.local], Optional[py.path.local], Dict[str, Union[str, List[str]]], ]

(Note that the 2nd item is Optional[py.path.local])

Here's a snippet from determine_setup, which calls locate_config:

def determine_setup( inifile: Optional[str], args: List[str], rootdir_cmd_arg: Optional[str] = None, config: Optional["Config"] = None, ) -> Tuple[py.path.local, Optional[str], Dict[str, Union[str, List[str]]]]: rootdir = None dirs = get_dirs_from_args(args) if inifile: # (1) inicfg = load_config_dict_from_file(py.path.local(inifile)) or {} if rootdir_cmd_arg is None: rootdir = get_common_ancestor(dirs) else: # (2) ancestor = get_common_ancestor(dirs) rootdir, inifile, inicfg = locate_config([ancestor]) if rootdir is None and rootdir_cmd_arg is None: ... return rootdir, inifile, inicfg or {}

In 1, indeed inifile ends up as a string by the time of return, but not when we get into 2, where inifile is a py.path.local (because of the locate_config call).

Shouldn't mypy catch that?

Regardless, I think we should change the return value from determine_setup to always return a py.path.local.

nicoddemus · 2020-06-11T20:02:59Z

testing/logging/test_reporting.py

+    """
+    )
+    p.move(sub.join(p.basename))
+    os.chdir(sub.strpath)


Use monkeypatch.chdir as it is safer. 👍

ah yes, auto revert post-test execution i assume? thanks for the tip

nicoddemus · 2020-06-11T20:03:02Z

testing/logging/test_reporting.py

+    """
+    )
+    p.move(sub.join(p.basename))
+    os.chdir(sub.strpath)


Use monkeypatch.chdir as it is safer. 👍

TIL, thanks

nicoddemus · 2020-06-11T20:03:07Z

testing/logging/test_reporting.py

+    """
+    )
+    p.move(sub.join(p.basename))
+    os.chdir(sub.strpath)


Use monkeypatch.chdir as it is safer. 👍

RonnyPfannschmidt · 2020-06-11T20:23:16Z

changelog/7336.bugfix.rst

@@ -0,0 +1 @@
+The pytest ``--log-file`` cli or ``log_file`` ini marker is now relative to the configs ``inifile`` directory.


this is a breaking change, the filename shoudld reflect that

Hmm good point, although the docs say otherwise, indeed this might break users.

nicoddemus · 2020-06-11T22:39:37Z

changelog/7336.bugfix.rst

@@ -0,0 +1 @@
+The pytest ``--log-file`` cli or ``log_file`` ini marker is now relative to the configs ``inifile`` directory.


Suggested change

The pytest ``--log-file`` cli or ``log_file`` ini marker is now relative to the configs ``inifile`` directory.

The pytest ``--log-file`` cli or ``log_file`` ini marker is now relative to the ``inifile`` directory, as it was always the intention. It was originally introduced as relative to the current working directory unintentionally.

nicoddemus · 2020-06-12T11:47:56Z

src/_pytest/config/findpaths.py

    rootdir_cmd_arg: Optional[str] = None,
    config: Optional["Config"] = None,
-) -> Tuple[py.path.local, Optional[str], Dict[str, Union[str, List[str]]]]:
+) -> Tuple[py.path.local, Optional[py.path.local], Dict[str, Union[str, List[str]]]]:


Please make sure to convert inifile to a py.path.local inside if inifile: below to honor this type annotation.

I have a PR locally which addresses these issues (by fixing errors after running mypy with pytest-dev/py#232). I'll post it in a bit.

test_inifilename hates such change(s) - investigating; maybe the test should be just updated:

'../../foo/bar.ini' # becomes: C:\Users\sy\AppData\Local\Temp\pytest-of-sy\pytest-972\test_inifilename0\foo\bar.ini

bluetech

Formally registering two issues I mentioned above:

This makes log_file relative to inidir, however it also affects the --log-file command line argument, where it is expected to be relative to the invocation dir, IMO.
The set_log_path is using rootdir as the base path for relative input, this seems inconsistent (maybe it's OK though).

symonk · 2020-06-12T17:33:13Z

Thanks @bluetech I will revisit based on your insights

symonk · 2020-06-17T17:16:18Z

Sorry been busy, going to look at this again; I will assume --log-file should remain untouched and only the ini marker for log_file should be relative to inifile

symonk added 3 commits June 11, 2020 01:28

Make the --log-file relative to inifile if it exists

d5c8ef8

remove unnecessary None default

afaacc5

Fix nested inidir lookup on cfg; fix spacing of comment

951a639

symonk marked this pull request as ready for review June 11, 2020 13:13

symonk requested a review from nicoddemus June 11, 2020 19:12

bluetech reviewed Jun 11, 2020

View reviewed changes

nicoddemus requested changes Jun 11, 2020

View reviewed changes

RonnyPfannschmidt reviewed Jun 11, 2020

View reviewed changes

nicoddemus reviewed Jun 11, 2020

View reviewed changes

relative --log-line PR feedback improvements

45d6a10

symonk force-pushed the log-file-relative-to-inifile-7336 branch from dd98247 to 45d6a10 Compare June 11, 2020 23:09

nicoddemus requested changes Jun 12, 2020

View reviewed changes

convert inifile to path.local when it is a string

bd4a57f

bluetech mentioned this pull request Jun 12, 2020

More type annotations, fix some typing bugs #7358

Merged

bluetech requested changes Jun 12, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

nicoddemus mentioned this pull request Jul 8, 2020

--log-file should create subdirectories #7467

Closed

This comment was marked as resolved.

Sign in to view

The-Compiler mentioned this pull request Sep 29, 2020

inaccurate documentation #7336

Closed

Base automatically changed from master to main March 9, 2021 20:40

pralkarz mentioned this pull request Feb 19, 2022

Correct log_file's inconsistent behavior #9700

Closed

Zac-HD mentioned this pull request Apr 21, 2022

Fix wrong log_file docs #9878

Merged

nicoddemus closed this in #9878 Apr 22, 2022

		result.assert_outcomes(passed=1)


		def test_log_file_is_in_pytest_ini_rootdir(testdir):

		@@ -0,0 +1 @@
		The pytest ``--log-file`` cli or ``log_file`` ini marker is now relative to the configs ``inifile`` directory.

Uh oh!

Conversation

symonk commented Jun 11, 2020 • edited by nicoddemus Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnikonorov commented Jun 11, 2020

Uh oh!

bluetech left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

symonk Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bluetech commented Jun 11, 2020

Uh oh!

nicoddemus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicoddemus Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

symonk Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

symonk Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicoddemus Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicoddemus Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

symonk Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bluetech left a comment

Choose a reason for hiding this comment

Uh oh!

symonk commented Jun 12, 2020

Uh oh!

symonk commented Jun 11, 2020 •

edited by nicoddemus

Loading

symonk Jun 11, 2020 •

edited

Loading

nicoddemus Jun 11, 2020 •

edited

Loading

symonk Jun 11, 2020 •

edited

Loading

symonk Jun 11, 2020 •

edited

Loading

nicoddemus Jun 12, 2020 •

edited

Loading

nicoddemus Jun 11, 2020 •

edited

Loading

symonk Jun 12, 2020 •

edited

Loading