Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions config_root/config/config_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@
import re
from typing import Any, Iterable, List, Optional

import config_root.config.config_ as crococon
import pandas as pd

import config_root.config.config_ as crococon
import helpers.hdbg as hdbg
import helpers.hdict as hdict
import helpers.hdocker as hdocker
Expand Down Expand Up @@ -86,7 +86,6 @@ def replace_shared_dir_paths(
return new_config


# TODO(gp): Add unit tests.
def sort_config_string(txt: str) -> str:
"""
Sort a string representing a Config in alphabetical order by the first
Expand Down
127 changes: 124 additions & 3 deletions config_root/config/test/test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -1864,7 +1864,6 @@ def test1(self) -> None:
key2:
key3:
key4:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you remove these empty lines or was that Linter? Here and also twice below

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I didn't, it was the linter.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls let's revert these Linter changes. I'll file an issue because it's some erroneous formatting

"""
should_be_pickleable_before = True
force_values_to_string = True
Expand All @@ -1886,7 +1885,6 @@ def test2(self) -> None:
key2:
key3:
key4:

"""
should_be_pickleable_before = False
force_values_to_string = True
Expand Down Expand Up @@ -2085,7 +2083,6 @@ def test5(self) -> None:
def test6(self) -> None:
"""
Test debug mode with `marked_as_used` == True.

This is a smoketest since the output of stacktrace is unstable.
"""
# Set multiline string value.
Expand Down Expand Up @@ -2496,3 +2493,127 @@ def test3(self) -> None:
key1 (marked_as_used=False, writer=None, val_type=str): hello.txt
"""
self.execute_stmt(stmt, exp, mode, globals())


# #############################################################################
# TestSortConfigString
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file where this class is located is wrong. Since the original function is in config_utils.py, its test should be in test_config_utils.py

# #############################################################################


class TestSortConfigString(hunitest.TestCase):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test cases themselves look good. Feel free to fix the bug in the function and you can use these tests to confirm that the fix works.

"""
Test sort_config_string function with various config formats.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls use backticks for the function name

"""

def check_sort_test(self, txt: str, expected: str) -> None:
"""
Helper method to test sorted config output.
"""
result = cconfig.sort_config_string(txt)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By convention, we call it actual

self.assertEqual(result, expected, fuzzy_match=True)

def test1(self) -> None:
"""
Test sorting of single-line config entries.
"""
txt = """
z_config: value3
a_config: value1
m_config: value2
"""
expected = """
a_config: value1
m_config: value2
z_config: value3
"""
self.check_sort_test(txt, expected)

def test2(self) -> None:
"""
Test sorting of multi-level config entries.
"""
txt = """
build_model:
activation: sigmoid
layers: 3
build_data:
source: database
type: timeseries
"""
expected = """
build_data:
source: database
type: timeseries
build_model:
activation: sigmoid
layers: 3
"""
self.check_sort_test(txt, expected)

def test3(self) -> None:
"""
Test handling of different indentation patterns and space combinations.
"""
txt = """
config1:
normal_indent: value1
wrong_indent: value2
config2:
extra_indent: value3
single: value4
bad_indent: value5
Comment on lines +2560 to +2564
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it expected that wrong_indent and bad_indent get removed? My understanding was that the function is only about sorting alphabetically. Does it also validate indentation? Maybe it can be added to the docstring in that case.

"""
expected = """
config1:
normal_indent: value1
config2:
extra_indent: value3
single value4
"""
self.check_sort_test(txt, expected)

def test4(self) -> None:
"""
Test sorting of complex nested structure with multiple levels.
"""
txt = """
z_parent:
child2:
key1: val1
key2: val2
child1: value
a_parent:
nested:
deep:
key: value
simple: test
"""
expected = """
a_parent:
nested:
deep:
key: value
simple: test
z_parent:
child1: value
child2:
key1: val1
key2: val2
"""
self.check_sort_test(txt, expected)

def test5(self) -> None:
"""
Test sorting with special characters in config names.
"""
txt = """
_special: value
#comment: note
@config: test
"""
expected = """
#comment: note
@config: test
_special: value
"""
self.check_sort_test(txt, expected)
Loading