From 31592bb121943b535ad7604b9b60850082050859 Mon Sep 17 00:00:00 2001 From: darrenhuai <60621295+darrenhuai@users.noreply.github.com> Date: Sun, 19 Jul 2026 16:45:10 -0700 Subject: [PATCH 1/3] test: add unit tests for rez.utils.base26 get_next_base26 and create_unique_base26_symlink had no dedicated tests. Covers the letter-increment/rollover logic directly, and the symlink creation/collision-retry logic via mocks (real symlink creation needs elevated privileges on some platforms, e.g. Windows without Developer Mode, so the filesystem boundary is mocked instead). Refs #2092 Signed-off-by: darrenhuai <60621295+darrenhuai@users.noreply.github.com> --- src/rez/tests/test_utils_base26.py | 112 +++++++++++++++++++++++++++++ 1 file changed, 112 insertions(+) create mode 100644 src/rez/tests/test_utils_base26.py diff --git a/src/rez/tests/test_utils_base26.py b/src/rez/tests/test_utils_base26.py new file mode 100644 index 0000000000..7eeaa2bb54 --- /dev/null +++ b/src/rez/tests/test_utils_base26.py @@ -0,0 +1,112 @@ +# SPDX-License-Identifier: Apache-2.0 +# Copyright Contributors to the Rez Project + + +""" +test rez.utils.base26 +""" +import errno +import os.path +from unittest.mock import patch + +from rez.utils.base26 import create_unique_base26_symlink, get_next_base26 +from rez.tests.util import TestBase + + +class TestGetNextBase26(TestBase): + def test_no_prev_returns_a(self) -> None: + self.assertEqual(get_next_base26(None), 'a') + self.assertEqual(get_next_base26(''), 'a') + + def test_increments_last_letter(self) -> None: + self.assertEqual(get_next_base26('a'), 'b') + self.assertEqual(get_next_base26('y'), 'z') + self.assertEqual(get_next_base26('ab'), 'ac') + + def test_rolls_over_to_new_letter(self) -> None: + self.assertEqual(get_next_base26('z'), 'aa') + self.assertEqual(get_next_base26('az'), 'ba') + self.assertEqual(get_next_base26('zz'), 'aaa') + + def test_rolls_over_multiple_letters(self) -> None: + self.assertEqual(get_next_base26('azz'), 'baa') + + def test_invalid_input_raises(self) -> None: + for bad in ('A', 'a1', 'a-b', ' a', 'a '): + with self.assertRaises(ValueError): + get_next_base26(bad) + + +class TestCreateUniqueBase26Symlink(TestBase): + """These mock the filesystem boundary (os.listdir/os.path.islink/os.symlink) + rather than creating real symlinks, since symlink creation requires elevated + privileges on some platforms (e.g. Windows without Developer Mode). + """ + + def test_returns_existing_symlink_if_already_pointing_at_source(self) -> None: + with patch( + "rez.utils.base26.find_matching_symlink", return_value="c" + ), patch("os.symlink") as symlink: + result = create_unique_base26_symlink("/pkgs", "/source/1.0") + + self.assertEqual(result, os.path.join("/pkgs", "c")) + symlink.assert_not_called() + + def test_creates_first_symlink_when_none_exist(self) -> None: + with patch( + "rez.utils.base26.find_matching_symlink", return_value=None + ), patch("os.listdir", return_value=[]), patch( + "os.symlink" + ) as symlink: + result = create_unique_base26_symlink("/pkgs", "/source/1.0") + + symlink.assert_called_once_with("/source/1.0", result) + self.assertTrue(result.endswith('a')) + + def test_creates_symlink_after_highest_existing(self) -> None: + with patch( + "rez.utils.base26.find_matching_symlink", return_value=None + ), patch( + "os.listdir", return_value=['a', 'b', 'c'] + ), patch( + "os.path.islink", return_value=True + ), patch("os.symlink") as symlink: + result = create_unique_base26_symlink("/pkgs", "/source/2.0") + + symlink.assert_called_once_with("/source/2.0", result) + self.assertTrue(result.endswith('d')) + + def test_retries_on_race_condition_then_succeeds(self) -> None: + exists_error = OSError(errno.EEXIST, "File exists") + + with patch( + "rez.utils.base26.find_matching_symlink", return_value=None + ), patch("os.listdir", return_value=[]), patch( + "os.symlink", side_effect=[exists_error, None] + ) as symlink: + result = create_unique_base26_symlink("/pkgs", "/source/1.0") + + self.assertEqual(symlink.call_count, 2) + self.assertTrue(result.endswith('a')) + + def test_reraises_non_eexist_oserror(self) -> None: + other_error = OSError(errno.EACCES, "Permission denied") + + with patch( + "rez.utils.base26.find_matching_symlink", return_value=None + ), patch("os.listdir", return_value=[]), patch( + "os.symlink", side_effect=other_error + ): + with self.assertRaises(OSError): + create_unique_base26_symlink("/pkgs", "/source/1.0") + + def test_gives_up_after_too_much_contention(self) -> None: + exists_error = OSError(errno.EEXIST, "File exists") + + with patch( + "rez.utils.base26.find_matching_symlink", return_value=None + ), patch("os.listdir", return_value=[]), patch( + "os.symlink", side_effect=exists_error + ): + with self.assertRaises(RuntimeError): + create_unique_base26_symlink("/pkgs", "/source/1.0") From 22ab8f47fd78fd5e8b2c7e72d5fe9935b6eb0e1e Mon Sep 17 00:00:00 2001 From: darrenhuai <60621295+darrenhuai@users.noreply.github.com> Date: Mon, 3 Aug 2026 19:34:51 -0700 Subject: [PATCH 2/3] fix: base26 length-then-lex ordering and reject trailing newlines Review feedback on the new base26 tests pointed out two real bugs that full coverage was masking: - create_unique_base26_symlink used max(names), which sorts lexicographically, so 'z' would beat 'aa' even though 'aa' is the later id. Added a mixed-length regression test and switched to max(names, key=lambda x: (len(x), x)). - get_next_base26 used re.match, which lets '$' match before a trailing newline, so inputs like 'a\n' slipped past validation. Switched to re.fullmatch and added newline cases to the invalid input test, wrapped in subTest per JeanChristopheMorinPerso's suggestion. Signed-off-by: darrenhuai <60621295+darrenhuai@users.noreply.github.com> --- src/rez/tests/test_utils_base26.py | 22 +++++++++++++++++++--- src/rez/utils/base26.py | 6 ++++-- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src/rez/tests/test_utils_base26.py b/src/rez/tests/test_utils_base26.py index 7eeaa2bb54..0e102160bc 100644 --- a/src/rez/tests/test_utils_base26.py +++ b/src/rez/tests/test_utils_base26.py @@ -32,9 +32,10 @@ def test_rolls_over_multiple_letters(self) -> None: self.assertEqual(get_next_base26('azz'), 'baa') def test_invalid_input_raises(self) -> None: - for bad in ('A', 'a1', 'a-b', ' a', 'a '): - with self.assertRaises(ValueError): - get_next_base26(bad) + for bad in ('A', 'a1', 'a-b', ' a', 'a ', 'a\n', 'ab\n'): + with self.subTest(item=bad): + with self.assertRaises(ValueError): + get_next_base26(bad) class TestCreateUniqueBase26Symlink(TestBase): @@ -76,6 +77,21 @@ def test_creates_symlink_after_highest_existing(self) -> None: symlink.assert_called_once_with("/source/2.0", result) self.assertTrue(result.endswith('d')) + def test_creates_symlink_after_highest_existing_with_mixed_lengths(self) -> None: + # regression test: a naive max(names) sorts lexicographically, so 'z' + # would incorrectly be picked over 'aa' as the "highest" name + with patch( + "rez.utils.base26.find_matching_symlink", return_value=None + ), patch( + "os.listdir", return_value=['a', 'b', 'c', 'z', 'aa'] + ), patch( + "os.path.islink", return_value=True + ), patch("os.symlink") as symlink: + result = create_unique_base26_symlink("/pkgs", "/source/3.0") + + symlink.assert_called_once_with("/source/3.0", result) + self.assertTrue(result.endswith('ab')) + def test_retries_on_race_condition_then_succeeds(self) -> None: exists_error = OSError(errno.EEXIST, "File exists") diff --git a/src/rez/utils/base26.py b/src/rez/utils/base26.py index ba286f1d97..1d52571b39 100644 --- a/src/rez/utils/base26.py +++ b/src/rez/utils/base26.py @@ -24,7 +24,7 @@ def get_next_base26(prev: str | None = None) -> str: return 'a' r = re.compile("^[a-z]*$") - if not r.match(prev): + if not r.fullmatch(prev): raise ValueError("Invalid base26") if not prev.endswith('z'): @@ -60,7 +60,9 @@ def create_unique_base26_symlink(path: str, source: str) -> str: ] if names: - prev = max(names) + # sort by length first, then alphabetically, so e.g. 'aa' correctly + # counts as higher than 'z' + prev = max(names, key=lambda x: (len(x), x)) else: prev = None From e4690dc4c03e2e9b28f5811fd220afcb800cec38 Mon Sep 17 00:00:00 2001 From: darrenhuai <60621295+darrenhuai@users.noreply.github.com> Date: Sat, 22 Aug 2026 19:26:25 -0700 Subject: [PATCH 3/3] test: make the race-retry test prove the retry advances the ID os.listdir was pinned to [] for the whole test, so after losing the race for 'a' the retry recomputed prev=None and picked 'a' again - the assertions held whether or not the loop re-read the directory. listdir now returns [] then ['a'], and the test checks the first attempt was 'a' and the surviving link is 'b'. Signed-off-by: darrenhuai <60621295+darrenhuai@users.noreply.github.com> --- src/rez/tests/test_utils_base26.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/rez/tests/test_utils_base26.py b/src/rez/tests/test_utils_base26.py index 0e102160bc..aaa1b2bf40 100644 --- a/src/rez/tests/test_utils_base26.py +++ b/src/rez/tests/test_utils_base26.py @@ -95,15 +95,22 @@ def test_creates_symlink_after_highest_existing_with_mixed_lengths(self) -> None def test_retries_on_race_condition_then_succeeds(self) -> None: exists_error = OSError(errno.EEXIST, "File exists") + # the first pass finds an empty dir and loses the race for 'a'; by the + # time it retries, the winner's 'a' is on disk, so it must move to 'b' with patch( "rez.utils.base26.find_matching_symlink", return_value=None - ), patch("os.listdir", return_value=[]), patch( + ), patch("os.listdir", side_effect=[[], ['a']]), patch( + "os.path.islink", return_value=True + ), patch( "os.symlink", side_effect=[exists_error, None] ) as symlink: result = create_unique_base26_symlink("/pkgs", "/source/1.0") self.assertEqual(symlink.call_count, 2) - self.assertTrue(result.endswith('a')) + self.assertEqual( + symlink.call_args_list[0][0][1], os.path.join("/pkgs", "a")) + symlink.assert_called_with("/source/1.0", result) + self.assertTrue(result.endswith('b')) def test_reraises_non_eexist_oserror(self) -> None: other_error = OSError(errno.EACCES, "Permission denied")