test: add unit tests for rez.utils.base26 - #2156
Conversation
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 AcademySoftwareFoundation#2092 Signed-off-by: darrenhuai <60621295+darrenhuai@users.noreply.github.com>
|
|
|
Hi @darrenhuai , thank you for the testing contribs. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2156 +/- ##
==========================================
+ Coverage 61.29% 61.34% +0.04%
==========================================
Files 164 164
Lines 20568 20568
Branches 3575 3575
==========================================
+ Hits 12607 12617 +10
+ Misses 7089 7081 -8
+ Partials 872 870 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Yes, both PRs were AI-assisted. Claude assisted in writing the test code, and drafting the commit messages and PR descriptions. I reviewed the diffs and the test/lint output before pushing each one and made the call on what to ship. Happy to add a disclosure to the PR title, is there a format you'd prefer, since the policy isn't finalized yet? Thank you! |
|
@darrenhuai Something as simple as what I'm putting on my PRs is fine for now, like this, in the PR description (not the title):
Just to explain (as a point of reference), we're not barring LLM-generated code, it's just so that we the maintainers can:
|
|
perfect thank you! will do so |
JeanChristopheMorinPerso
left a comment
There was a problem hiding this comment.
I threw a coding agent at your PR and it found two bugs in base26.py that would be very easy to fix in this PR if you want. I don't consider these blocking as they are probably corner cases.
| "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')) |
There was a problem hiding this comment.
| "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')) | |
| "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/2.0") | |
| symlink.assert_called_once_with("/source/2.0", result) | |
| self.assertTrue(result.endswith('ab')) |
Doing this will highlight that there's actually a bug in create_unique_base26_symlink. max(names) should be replaced with max(names, key=lambda x: (len(x), x)).
That's because:
>>> max(['z', 'aa'])
'z'In other words, the tests have full coverage purely in terms of code, but not in terms of logic.
| for bad in ('A', 'a1', 'a-b', ' a', 'a '): | ||
| with self.assertRaises(ValueError): | ||
| get_next_base26(bad) |
There was a problem hiding this comment.
| 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 '): | |
| with self.subTest(item=bad): | |
| with self.assertRaises(ValueError): | |
| get_next_base26(bad) |
This will more clearly show that we are testing multiple things.
ALso, note how there's a but in the get_next_base26 where it accepts a trailing \n. We should replace match with fullmatch.
rez.utils.base26 had no tests at all - get_next_base26 (the letter-increment logic for variant shortlink IDs) and create_unique_base26_symlink were both uncovered.
Tests for get_next_base26 cover the increment/rollover cases directly (a->b, z->aa, az->ba, zz->aaa, etc) and invalid input. For create_unique_base26_symlink I mocked the filesystem calls (os.listdir, os.symlink, find_matching_symlink) instead of creating real symlinks, since that needs elevated privileges on Windows without Developer Mode - this still covers the existing-match short circuit, picking the next id after the highest existing one, the EEXIST retry-on-race path, non-EEXIST errors propagating, and giving up after too much contention.
Ran the full suite locally (332 passed), flake8 and ruff clean on the new file.
Refs #2092
Disclosure: Claude (Sonnet 5) was used to assist in writing the test code and drafting this commit message and PR description.