tests: Wrap or remove deprecated functions #760

pdgendt · 2024-10-23T07:09:35Z

Make deprecated calls explicit during testing or remove them if unnecessary.

This allows us to promote warnings to errors with pytest.

marc-hb · 2024-10-23T17:12:22Z

tests/test_config.py

    return cp

+def update_config(*args, **kwargs):
+    # For testing purposes it's easiest to use the deprecated functions


Nit

Suggested change

# For testing purposes it's easiest to use the deprecated functions

# For testing purposes it's easier to use the deprecated functions

marc-hb · 2024-10-23T17:20:44Z

tests/test_config.py

@@ -29,10 +28,22 @@ def autouse_config_tmpdir(config_tmpdir):

 def cfg(f=ALL, topdir=None):
    # Load a fresh configuration object at the given level, and return it.
-    cp = configparser.ConfigParser(allow_no_value=True)
-    config.read_config(configfile=f, config=cp, topdir=topdir)
+    cp = config.configparser.ConfigParser(allow_no_value=True)


Mostly not your fault but this commit is incredibly hard to review... first, this import rename is horrible because it hides both the origin and nature of config:

from west import configuration as config

(don't change it now because it would have a massive ripple effect)

Then, I can't find west.configuration.configparser in the documentation. Is this is some "private" object (ab)used for "white box" testing? If yes please add a comment stating that. If not, sorry I missed what it is.

The icing on the cake is the fact that this commit replaces Python's built-in configparser with west.configuration.configparser which has the very same name while being a bit of a mystery...

I hate to ask but... could you slow down a little bit? Maybe move this to a separate commit. Either way, provide a bit more comments, commit message, etc. Thanks in advance!

I was contemplating the same things, but tried to touch it "minimally" I'll try to clean this up so that it makes sense what is builtin and what not.

I've reverted to the original configparser.

marc-hb · 2024-10-23T17:21:55Z

tests/test_config.py

+    # For testing purposes it's easiest to use the deprecated functions
+    # but make these calls explicit.
+    with pytest.deprecated_call():
+        return config.update_config(*args, **kwargs)


marc-hb · 2024-10-23T17:27:36Z

tests/test_project.py

@@ -1631,7 +1630,6 @@ def test_init_local_with_manifest_filename(repos_tmpdir):
    # success
    cmd(['init', '--mf', 'project.yml', '-l', zephyr_install_dir])
    workspace.chdir()
-    config.read_config()


Should this be a separate commit?

Moved to a separate commit.

marc-hb · 2024-10-23T17:27:54Z

tox.ini

@@ -37,6 +37,6 @@ setenv =
    # For instance: ./.tox/py3/tmp/
    TOXTEMPDIR={envtmpdir}
 commands =
-  python -m pytest --cov-report=html --cov=west {posargs:tests} --basetemp='{envtmpdir}/pytest with space/'
+  python -m pytest -W error --cov-report=html --cov=west {posargs:tests} --basetemp='{envtmpdir}/pytest with space/'


Nice, but could there be a convenient way/setting to temporarily turn this off? I mean without git diff and git status pollution.

When hacking, there is often a intermediate stage where you really want the tests to pass on work in progress that still has warnings.

I've removed it from the tox.ini file and added it to the CI run.

marc-hb · 2024-10-23T17:33:00Z

tests/test_config.py

+    with pytest.deprecated_call():
+        return config.update_config(*args, **kwargs)
+
+def delete_config(*args, **kwargs):


Warning: this review comment will be hard to find because that line is changed in a later commit in the PR and Github doesn't like the multi-commit, force-push workflow.

There is already an API named delete_config(), I know because it shows up in a later commit! So please use a different name for internal symbols. Example:

Suggested change

def delete_config(*args, **kwargs):

def delete_testcfg(*args, **kwargs):

Same for update_config above.

Sincere condolences for the ripple effect on consecutive commits :-(

Generally speaking, please choose less "API-looking" names for internal symbols. Pretty much everything here is called something_config and it's incredibly hard to keep track of what is: 1) Builtin Python (e.g. configparser) 2) deprecated west API 3) current west API 4) private west objects abused for "white box" testing 5) local/private test symbols,....

It's never going to be perfect and please avoid unnecessary git churn when possible, but brand new symbols like this one is where progress can and should be made.

This is a cleanup PR so it should bring more clarity.

Yeah, I tried to do it in some reasonable steps, but I think it can make it harder to follow.
I'll come back to this PR, but there's no urgency here, as I didn't intend on getting into 1.3.

Renamed as suggested, which is clearer.

These calls are deprecated and unneeded, simply remove them. Signed-off-by: Pieter De Gendt <[email protected]>

stale review

marc-hb

Looks great except for one confusing, one-line change.

marc-hb · 2024-10-28T22:28:43Z

tests/test_config.py

@@ -30,9 +32,21 @@ def autouse_config_tmpdir(config_tmpdir):
 def cfg(f=ALL, topdir=None):
    # Load a fresh configuration object at the given level, and return it.
    cp = configparser.ConfigParser(allow_no_value=True)
-    config.read_config(configfile=f, config=cp, topdir=topdir)
+    cp.read(config._gather_configs(f, topdir), encoding='utf-8')


After searching the code, I found that _gather_configs() is just an internal implementation detail of read_config() and used only by that deprecated function. This means _gather_configs() is effectively deprecated too. So I don't see what this particular line change brings except some confusion, sorry.

I think you should either "upgrade" this function to the newer API if possible, or not change it at all yet if not possible and/or too much work.

I've wrapped the call in with pytest.deprecated_call() as it requires a lot more work in this case. Also added a TODO comment.

Make deprecated calls explicit during testing or remove them if unnecessary. Change the update_config and delete_config helper methods to use a Configuration class instead of the global deprecated config functions. Signed-off-by: Pieter De Gendt <[email protected]>

The deprecated delete_config function supports passing a list of ConfigFile enumerations. Update argument typing accordingly. Signed-off-by: Pieter De Gendt <[email protected]>

Don't allow warnings during CI testing. Signed-off-by: Pieter De Gendt <[email protected]>

marc-hb previously requested changes Oct 23, 2024

View reviewed changes

tests: Remove deprecated calls to read_config

265949d

These calls are deprecated and unneeded, simply remove them. Signed-off-by: Pieter De Gendt <[email protected]>

pdgendt force-pushed the pytest-warnings branch 2 times, most recently from d450aad to 6aa9f89 Compare October 26, 2024 13:52

pdgendt requested a review from marc-hb October 26, 2024 14:06

marc-hb reviewed Oct 28, 2024

View reviewed changes

pdgendt added 3 commits October 29, 2024 11:06

configuration: Update arguments type for delete_config

5da5735

The deprecated delete_config function supports passing a list of ConfigFile enumerations. Update argument typing accordingly. Signed-off-by: Pieter De Gendt <[email protected]>

ci: tox: Turn pytest warnings into errors

21a4d02

Don't allow warnings during CI testing. Signed-off-by: Pieter De Gendt <[email protected]>

pdgendt force-pushed the pytest-warnings branch from 6aa9f89 to 21a4d02 Compare October 29, 2024 10:07

marc-hb approved these changes Oct 30, 2024

View reviewed changes

pdgendt merged commit 531e330 into zephyrproject-rtos:main Oct 30, 2024
16 checks passed

pdgendt deleted the pytest-warnings branch October 30, 2024 08:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: Wrap or remove deprecated functions #760

tests: Wrap or remove deprecated functions #760

pdgendt commented Oct 23, 2024

marc-hb Oct 23, 2024

marc-hb Oct 23, 2024

pdgendt Oct 23, 2024

pdgendt Oct 26, 2024

marc-hb Oct 23, 2024

marc-hb Oct 23, 2024

pdgendt Oct 26, 2024

marc-hb Oct 23, 2024

pdgendt Oct 26, 2024

marc-hb Oct 23, 2024 •

edited

Loading

pdgendt Oct 23, 2024

pdgendt Oct 26, 2024

marc-hb left a comment

marc-hb Oct 28, 2024

pdgendt Oct 29, 2024

	# For testing purposes it's easiest to use the deprecated functions
	# For testing purposes it's easier to use the deprecated functions

	def delete_config(args, *kwargs):
	def delete_testcfg(args, *kwargs):

tests: Wrap or remove deprecated functions #760

tests: Wrap or remove deprecated functions #760

Conversation

pdgendt commented Oct 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb Oct 23, 2024 •

edited

Loading