Adds tests for the current state of the project when using TOML #324

kingbuzzman · 2025-06-25T12:58:38Z

Adds tests for the current state of the application, no logic changes

Increases the coverage from 85% (master) to 91%

Copilot

Pull Request Overview

This PR adds tests to improve the project's TOML configuration parsing while increasing overall test coverage. Key changes include:

New test classes for TOML and composite configuration parsing.
Additional tests to validate behavior with various TOML file conditions.
An update to setup.py to include the toml package in test dependencies.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
tests/test_configargparse.py	Adds new tests for TOML parsing and composite configuration error handling.
setup.py	Updates test requirements to include the toml package.

Comments suppressed due to low confidence (1)

tests/test_configargparse.py:1895

[nitpick] Consider adding an inline comment to explain why appending extra keys in the TOML file is expected to trigger a SystemExit, which will help clarify the test intent.

    def test_toml_extra(self):

Copilot · 2025-06-26T12:47:58Z

tests/test_configargparse.py

+    def write_yaml_file(self, content="[section]\nkey1: yaml1\nkey2: yaml2"):
+        with open("config.yaml", "w") as f:
+            f.write(content)
+
+    def write_ini_file(self, content="[section]\nkey1=ini1\nkey2=ini2"):
+        with open("config.ini", "w") as f:
+            f.write(content)
+
+    def write_toml_file(self, content="[section]\nkey1 = 'toml1'\nkey2 = 'toml2'"):
+        with open("config.toml", "w") as f:
+            f.write(content)


[nitpick] Consider refactoring the repeated file writing helper methods (for YAML, INI, and TOML) into a shared utility to reduce duplication across tests.

Suggested change

def write_yaml_file(self, content="[section]\nkey1: yaml1\nkey2: yaml2"):

with open("config.yaml", "w") as f:

f.write(content)

def write_ini_file(self, content="[section]\nkey1=ini1\nkey2=ini2"):

with open("config.ini", "w") as f:

f.write(content)

def write_toml_file(self, content="[section]\nkey1 = 'toml1'\nkey2 = 'toml2'"):

with open("config.toml", "w") as f:

f.write(content)

def write_config_file(self, filename, content):

"""Writes the given content to the specified file."""

with open(filename, "w") as f:

f.write(content)

def write_yaml_file(self, content="[section]\nkey1: yaml1\nkey2: yaml2"):

self.write_config_file("config.yaml", content)

def write_ini_file(self, content="[section]\nkey1=ini1\nkey2=ini2"):

self.write_config_file("config.ini", content)

def write_toml_file(self, content="[section]\nkey1 = 'toml1'\nkey2 = 'toml2'"):

self.write_config_file("config.toml", content)

Have you not heard of StringIO, copilot?

bw2

LGTM! @tristanlatr @tbooth any objections to merging this?

tbooth · 2025-06-26T14:34:31Z

tests/test_configargparse.py

+            default_config_files=['config.yaml', 'config.toml', 'config.ini'],
+            config_file_parser_class=configargparse.CompositeConfigParser(
+                [
+                    configargparse.TomlConfigParser(['section']),


If this line is removed the tests all still pass, so either remove the line or make it necessary.

Sort of. Issue is that IniConfigParser picks up the slack, since the two are structured very similarly. If you comment the other two out, and run the test for just pytest -s tests/test_configargparse.py::TestCompositeConfigParser::test_toml it passes.

Right, so just include something in the TOML snippet that would not be properly recognised by the INI parser, and that will make the test much more useful.

For that i would need to touch the code, there is a bug that will cause a crash raising ConfigFileParserException.

This is the best i can do without touching anything else, and focusing the changes VERY narrowly.

I plan on fixing this issue after the other toml PR

It's fine IMO for these new tests you are adding to fail with the current code. Adding correct but failing tests is the motivator for accepting the patches to fix them!

I'd suggest adding a line like foo=0xDEADBEEF to the sample TOML snippet in your test. The INI parser will parse this incorrectly as "0xDEADBEEF". The old toml module will (it appears) raise an exception. The newer tomli module you want to introduce will correctly parse it as 3735928559, which should (but will currently not) be converted to a string :"3735928559"

Yeah. I agree with what you say, but if its all the same, i'd like to do this in another PR. Can you approve it... if you deem it... worthy, so i can continue... pretty please

While youre at it, there are another 2 prs open as of 20mins ago, SUPER tiny. low risk. -- if you can take a quick look, id appreciate it immensely.

tbooth · 2025-06-26T14:44:15Z

tests/test_configargparse.py

+        """)
+        parser = configargparse.TomlConfigParser(['tool.section'])
+        with open('config.toml', 'r') as f:
+            self.assertEqual(parser.parse(f), {'key1': "toml1", 'key2': [1, 2, 3]})


As noted by @tristanlatr this behaviour is incorrect so please change it to 'key2': ['1', '2', '3'] and then this test should (correctly) fail. If we want to force a module release before fixing it then it can be marked as an expected failure.

I fixed this in my own fork by having a unified config data cleanup function, rather then relying on each separate parser class to do its own cleanup. I would advocate adopting this change to resolve the failure, rather then writing a new quick fix to the TOML parser.

tbooth · 2025-06-26T14:50:30Z

tests/test_configargparse.py



+class TestTomlConfigParser(unittest.TestCase):
+    def setUp(self):


This temp file creation is a lot of work compared to using io.StringIO. Also, changing directory within a unit test is best avoided unless strictly necessary. Not a major problem - I can sort this out in a follow-up PR.

Converted most..

tbooth · 2025-06-26T14:55:42Z

I added some comments but the one that really needs attention is the one on line 1834. The test purports to be testing composite parsing with the TOML parser but removing that parser from the list does not cause the tests to fail.

kingbuzzman · 2025-06-30T18:29:48Z

@tbooth anything missing from your perspective?

kingbuzzman · 2025-07-03T10:22:27Z

@tbooth thanks for the merge. Next time please squash my commits, there is no need for so much noise in the commit history in the master branch:

The major issue of not squashing is that now all the PRs have conflicts now...

Adds tests for the current state of the project when using TOML

60faa3a

kingbuzzman mentioned this pull request Jun 25, 2025

Updates toml library #317

Draft

kingbuzzman marked this pull request as draft June 25, 2025 13:00

Filters if toml is not installed

0cffbb9

kingbuzzman temporarily deployed to pypi June 25, 2025 13:07 — with GitHub Actions Inactive

kingbuzzman added 2 commits June 25, 2025 09:09

Adds better message

5a45b72

.

085ff18

kingbuzzman temporarily deployed to pypi June 25, 2025 13:12 — with GitHub Actions Inactive

I dont need this anymore..

7aebfc8

kingbuzzman temporarily deployed to pypi June 25, 2025 13:14 — with GitHub Actions Inactive

kingbuzzman marked this pull request as ready for review June 25, 2025 13:15

bw2 requested review from bw2 and Copilot June 26, 2025 12:47

Copilot AI reviewed Jun 26, 2025

View reviewed changes

bw2 approved these changes Jun 26, 2025

View reviewed changes

tbooth reviewed Jun 26, 2025

View reviewed changes

Formats & adds xfail

d284acd

kingbuzzman temporarily deployed to pypi June 26, 2025 15:18 — with GitHub Actions Inactive

kingbuzzman added 2 commits June 26, 2025 11:27

Converts more to stringIO

537a316

Fix for windows -- dumb windows

f193ed1

kingbuzzman temporarily deployed to pypi June 26, 2025 15:44 — with GitHub Actions Inactive

Slightly better..

868fed1

kingbuzzman temporarily deployed to pypi June 26, 2025 16:02 — with GitHub Actions Inactive

Merge branch 'bw2:master' into dev/current-state

4b99361

kingbuzzman temporarily deployed to pypi June 26, 2025 20:17 — with GitHub Actions Inactive

Expected errors

d621a0f

kingbuzzman temporarily deployed to pypi June 27, 2025 07:27 — with GitHub Actions Inactive

tbooth merged commit a7aaba3 into bw2:master Jul 1, 2025
21 checks passed

kingbuzzman deleted the dev/current-state branch July 1, 2025 22:35

kingbuzzman mentioned this pull request Jul 3, 2025

remove Python2 comptability code #291

Closed



		class TestTomlConfigParser(unittest.TestCase):
		def setUp(self):

Adds tests for the current state of the project when using TOML #324

Adds tests for the current state of the project when using TOML #324

Uh oh!

Conversation

kingbuzzman commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bw2 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!

kingbuzzman Jun 26, 2025 • 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!

tbooth Jun 26, 2025 • 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!

kingbuzzman Jun 26, 2025 • 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!

tbooth commented Jun 26, 2025

Uh oh!

kingbuzzman commented Jun 30, 2025

Uh oh!

Uh oh!

kingbuzzman commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kingbuzzman commented Jun 25, 2025 •

edited

Loading

kingbuzzman Jun 26, 2025 •

edited

Loading

tbooth Jun 26, 2025 •

edited

Loading

kingbuzzman Jun 26, 2025 •

edited

Loading

kingbuzzman commented Jul 3, 2025 •

edited

Loading