Generate more unique names for new book folders (BL-15608)#7683
Generate more unique names for new book folders (BL-15608)#7683JohnThomson wants to merge 4 commits intomasterfrom
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
JohnThomson
left a comment
JohnThomson
left a comment
There was a problem hiding this comment.
@JohnThomson made 3 comments.
Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @andrew-polk).
JohnThomson
force-pushed
the
uniqueBookNames
branch
from
ac9c4c7 to
f2c9f7c
Compare
JohnThomson
left a comment
JohnThomson
left a comment
There was a problem hiding this comment.
@JohnThomson made 2 comments.
Reviewable status: 0 of 4 files reviewed, 5 unresolved discussions (waiting on @andrew-polk).
JohnThomson
force-pushed
the
uniqueBookNames
branch
from
f2c9f7c to
4346384
Compare
andrew-polk
left a comment
andrew-polk
left a comment
There was a problem hiding this comment.
@andrew-polk reviewed all commit messages and resolved 1 discussion.
Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @JohnThomson).
JohnThomson
left a comment
JohnThomson
left a comment
There was a problem hiding this comment.
@JohnThomson made 4 comments.
Reviewable status: 0 of 4 files reviewed, 5 unresolved discussions (waiting on @andrew-polk).
src/BloomTests/Book/BookStorageTests.cs line 210 at r4 (raw file):
[Test] public void MoveBookToSafeName_NameNotNFC_NewNameConflicts_Moves()
This test (in addition to the one above) is not useful any more; name conflicts are vanishingly unlikely and hard to simulate.
src/BloomTests/Book/BookStorageTests.cs line 1136 at r4 (raw file):
[Test] public void SetBookName_FolderWithSanitizedNameAlreadyExists_AddsANumberToName()
This seems to be doing the same thing as the previous test, at least now that a conflict is so unlikely.
|
|
||
| var expectedFolder = bookFolder.Replace("e\x0301", "\x00e9"); | ||
|
|
||
| Assert.That(BookStorage.MoveBookToSafeName(bookFolder), Is.EqualTo(expectedFolder)); | ||
| Assert.That(Directory.Exists(expectedFolder)); | ||
| var expectedBookPath = bookPath.Replace("e\x0301", "\x00e9"); | ||
| Assert.That(File.ReadAllText(expectedBookPath), Is.EqualTo("this is a test")); | ||
| } | ||
|
|
||
| [Test] | ||
| public void MoveBookToSafeName_NameNotNFC_NewNameConflicts_Moves() | ||
| { | ||
| var oldName = "Some nice\x0301 book"; | ||
| var bookFolder = Path.Combine(_fixtureFolder.Path, oldName); | ||
| Directory.CreateDirectory(bookFolder); | ||
| var bookPath = Path.Combine(bookFolder, oldName + ".htm"); | ||
| File.WriteAllText(bookPath, "this is a test"); | ||
|
|
||
| var desiredFolder = bookFolder.Replace("e\x0301", "\x00e9"); | ||
| Directory.CreateDirectory(desiredFolder); | ||
| var expectedFolder = desiredFolder + " 2"; | ||
|
|
||
| Assert.That(BookStorage.MoveBookToSafeName(bookFolder), Is.EqualTo(expectedFolder)); | ||
| Assert.That(Directory.Exists(expectedFolder)); | ||
| var expectedBookPath = Path.Combine( | ||
| expectedFolder, | ||
| Path.GetFileName(expectedFolder) + ".htm" | ||
| ); | ||
| // Typically we added hyphen and a guid. Not sure if that's good. | ||
| var result = BookStorage.MoveBookToSafeName(bookFolder); | ||
| Assert.That(result, Does.StartWith(expectedFolder)); | ||
| Assert.That(Directory.Exists(result)); | ||
| var expectedBookPath = Path.Combine(result, Path.GetFileName(result) + ".htm"); | ||
| Assert.That(File.ReadAllText(expectedBookPath), Is.EqualTo("this is a test")); | ||
| } |
There was a problem hiding this comment.
🚩 Deleted test MoveBookToSafeName_NameNotNFC_NewNameConflicts_Moves reduces coverage
The test MoveBookToSafeName_NameNotNFC_NewNameConflicts_Moves (old lines 209-228) was deleted entirely rather than updated. This test specifically validated the case where the sanitized name conflicts with an existing folder. With the new always-add-GUID behavior, conflict is extremely unlikely, but the test still provided value in verifying that the conflict-resolution loop in GetUniqueBookFolderName works. Consider adding a unit test for GetUniqueBookFolderName directly that validates collision handling (where the first proposed name already exists as a directory).
(Refers to lines 192-209)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Since the code generates a new guid, there's no straightforward way to pre-arrange a conflict. We'd have to somehow reorganize the code so that the guid to use can be fed in. And then we are NOT really taking the same code path as when the 1 in 4 billion chance comes off and we get a conflict. I decided trying to test this corner was not a good use of time.
3 participants
This change is