refactor(pipeline): MP1b review - unify DatCollectionAdapter + TOC/log test gaps
Adversarially-verified review findings 7 and 8: (7) The DatCollection->IDatReaderWriter adapter existed as THREE near-identical copies (App-internal original, Bake's copy, Content. Tests' copy) — a structure where adapter drift is exactly what the live-vs-pak equivalence suite cannot detect (both sides would only drift together if they shared one implementation). Now ONE public AcDream.Content.DatCollectionAdapter next to IDatReaderWriter (GL-free home established in MP1a), carrying App's FULL behavior including the [dat-miss] TryGet tripwire log (which now also covers the bake tool and the equivalence suite) and the caching/locking. All three copies deleted; WbMeshAdapter (App), BakeRunner (Bake), and PakEquivalenceTests (Content.Tests) resolve the shared class. Iteration properties return the REAL dat iterations — the App copy's hardcoded 0 was a stub nothing read; the unification intentionally keeps truth (noted in the doc comment). Verified post-move: no Silk.NET anywhere in Content / Bake / Content.Tests / Bake.Tests resolved dependency graphs. (8) Two test gaps closed in PakRoundTripTests: (a) direct on-disk TOC sortedness — blobs added in DESCENDING key order, then the raw file bytes parsed (not through the reader) and every TOC entry asserted strictly ascending; (b) corrupt-blob logging — five repeated reads through both public paths (TryReadObjectMeshData + ContainsKey) with stderr captured, asserting exactly ONE [pak-corrupt] line for the victim key. Full suite: 4120 tests, 0 failures (Content.Tests 56, Bake.Tests 1, plus the pre-existing 4 skips).
This commit is contained in:
Erik
parent
86e0dc4655
commit
859cf5ec02
7 changed files with 135 additions and 351 deletions
|
|
@ -1,145 +0,0 @@
|
|||
using System;
|
||||
using System.Collections.Concurrent;
|
||||
using System.Collections.Generic;
|
||||
using System.Collections.ObjectModel;
|
||||
using System.Diagnostics.CodeAnalysis;
|
||||
using AcDream.Content;
|
||||
using DatReaderWriter;
|
||||
using DatReaderWriter.Enums;
|
||||
using DatReaderWriter.Lib.IO;
|
||||
|
||||
namespace AcDream.Content.Tests;
|
||||
|
||||
/// <summary>
|
||||
/// Adapts <see cref="DatCollection"/> to <see cref="IDatReaderWriter"/> for
|
||||
/// MeshExtractor's constructor, scoped to the equivalence test suite. A
|
||||
/// third copy alongside AcDream.App.Rendering.Wb.DatCollectionAdapter
|
||||
/// (internal to AcDream.App) and AcDream.Bake.BakeDatCollectionAdapter
|
||||
/// (internal to AcDream.Bake) — tests live in the project matching the
|
||||
/// layer under test (structure rule 6), so Content.Tests cannot reference
|
||||
/// AcDream.Bake just to borrow its copy. Plain dat-access glue, not an
|
||||
/// AC-specific algorithm.
|
||||
/// </summary>
|
||||
internal sealed class ContentTestDatCollectionAdapter : IDatReaderWriter {
|
||||
private readonly DatCollection _dats;
|
||||
private readonly TestDatDatabaseWrapper _portal;
|
||||
private readonly TestDatDatabaseWrapper _cell;
|
||||
private readonly TestDatDatabaseWrapper _highRes;
|
||||
private readonly TestDatDatabaseWrapper _language;
|
||||
private readonly ReadOnlyDictionary<uint, IDatDatabase> _cellRegions;
|
||||
|
||||
public ContentTestDatCollectionAdapter(DatCollection dats) {
|
||||
ArgumentNullException.ThrowIfNull(dats);
|
||||
_dats = dats;
|
||||
_portal = new TestDatDatabaseWrapper(dats.Portal);
|
||||
_cell = new TestDatDatabaseWrapper(dats.Cell);
|
||||
_highRes = new TestDatDatabaseWrapper(dats.HighRes);
|
||||
_language = new TestDatDatabaseWrapper(dats.Local);
|
||||
|
||||
var regions = new Dictionary<uint, IDatDatabase> { [0u] = _cell };
|
||||
_cellRegions = new ReadOnlyDictionary<uint, IDatDatabase>(regions);
|
||||
}
|
||||
|
||||
public string SourceDirectory => _dats.Options.DatDirectory ?? string.Empty;
|
||||
|
||||
public IDatDatabase Portal => _portal;
|
||||
public ReadOnlyDictionary<uint, IDatDatabase> CellRegions => _cellRegions;
|
||||
public IDatDatabase HighRes => _highRes;
|
||||
public IDatDatabase Language => _language;
|
||||
|
||||
public ReadOnlyDictionary<uint, uint> RegionFileMap =>
|
||||
new ReadOnlyDictionary<uint, uint>(new Dictionary<uint, uint>());
|
||||
|
||||
public int PortalIteration => _portal.Iteration;
|
||||
public int CellIteration => _cell.Iteration;
|
||||
public int HighResIteration => _highRes.Iteration;
|
||||
public int LanguageIteration => _language.Iteration;
|
||||
|
||||
public bool TryGetFileBytes(uint regionId, uint fileId, ref byte[] bytes, out int bytesRead) {
|
||||
return _dats.Cell.TryGetFileBytes(fileId, ref bytes, out bytesRead);
|
||||
}
|
||||
|
||||
public IEnumerable<IDatReaderWriter.IdResolution> ResolveId(uint id) {
|
||||
var results = new List<IDatReaderWriter.IdResolution>();
|
||||
|
||||
void CheckDb(TestDatDatabaseWrapper wrapper) {
|
||||
var rawDb = wrapper.RawDatabase;
|
||||
if (rawDb.Tree.TryGetFile(id, out _)) {
|
||||
var type = rawDb.TypeFromId(id);
|
||||
if (type != DBObjType.Unknown) {
|
||||
results.Add(new IDatReaderWriter.IdResolution(wrapper, type));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
CheckDb(_highRes);
|
||||
CheckDb(_portal);
|
||||
CheckDb(_language);
|
||||
CheckDb(_cell);
|
||||
|
||||
return results;
|
||||
}
|
||||
|
||||
public bool TrySave<T>(T obj, int iteration = 0) where T : IDBObj =>
|
||||
throw new NotSupportedException($"{nameof(ContentTestDatCollectionAdapter)} is read-only.");
|
||||
|
||||
public bool TrySave<T>(uint regionId, T obj, int iteration = 0) where T : IDBObj =>
|
||||
throw new NotSupportedException($"{nameof(ContentTestDatCollectionAdapter)} is read-only.");
|
||||
|
||||
public void Dispose() {
|
||||
// The underlying DatCollection is owned by the caller.
|
||||
}
|
||||
}
|
||||
|
||||
internal sealed class TestDatDatabaseWrapper : IDatDatabase {
|
||||
private readonly DatDatabase _db;
|
||||
private readonly ConcurrentDictionary<(Type, uint), IDBObj> _cache = new();
|
||||
private readonly object _lock = new();
|
||||
|
||||
public TestDatDatabaseWrapper(DatDatabase db) {
|
||||
ArgumentNullException.ThrowIfNull(db);
|
||||
_db = db;
|
||||
}
|
||||
|
||||
internal DatDatabase RawDatabase => _db;
|
||||
|
||||
public DatDatabase Db => _db;
|
||||
public int Iteration => _db.Iteration?.CurrentIteration ?? 0;
|
||||
|
||||
public IEnumerable<uint> GetAllIdsOfType<T>() where T : IDBObj => _db.GetAllIdsOfType<T>();
|
||||
|
||||
public bool TryGet<T>(uint fileId, [MaybeNullWhen(false)] out T value) where T : IDBObj {
|
||||
if (_cache.TryGetValue((typeof(T), fileId), out var cached)) {
|
||||
value = (T)cached;
|
||||
return true;
|
||||
}
|
||||
|
||||
lock (_lock) {
|
||||
if (_db.TryGet<T>(fileId, out value)) {
|
||||
_cache.TryAdd((typeof(T), fileId), value);
|
||||
return true;
|
||||
}
|
||||
}
|
||||
|
||||
return false;
|
||||
}
|
||||
|
||||
public bool TryGetFileBytes(uint fileId, [MaybeNullWhen(false)] out byte[] value) {
|
||||
lock (_lock) {
|
||||
return _db.TryGetFileBytes(fileId, out value);
|
||||
}
|
||||
}
|
||||
|
||||
public bool TryGetFileBytes(uint fileId, ref byte[] bytes, out int bytesRead) {
|
||||
lock (_lock) {
|
||||
return _db.TryGetFileBytes(fileId, ref bytes, out bytesRead);
|
||||
}
|
||||
}
|
||||
|
||||
public bool TrySave<T>(T obj, int iteration = 0) where T : IDBObj =>
|
||||
throw new NotSupportedException($"{nameof(TestDatDatabaseWrapper)} is read-only.");
|
||||
|
||||
public void Dispose() {
|
||||
// The underlying DatDatabase is owned by DatCollection.
|
||||
}
|
||||
}
|
||||
|
|
@ -42,7 +42,9 @@ public sealed class PakEquivalenceTests {
|
|||
if (datDir is null) return; // dats absent (CI) — skip, matching suite convention
|
||||
|
||||
using var dats = new DatCollection(datDir, DatAccessType.Read);
|
||||
using var datReaderWriter = new ContentTestDatCollectionAdapter(dats);
|
||||
// The unified AcDream.Content.DatCollectionAdapter — same class the
|
||||
// client and the bake tool use (MP1b review finding 7).
|
||||
using var datReaderWriter = new DatCollectionAdapter(dats);
|
||||
var logger = new TestConsoleLogger();
|
||||
|
||||
var sideStaged = new List<ObjectMeshData>();
|
||||
|
|
|
|||
|
|
@ -383,6 +383,79 @@ public class PakRoundTripTests : IDisposable {
|
|||
Assert.False(File.Exists(path));
|
||||
}
|
||||
|
||||
// ---- on-disk TOC sortedness (review finding 8) ---------------------------
|
||||
|
||||
[Fact]
|
||||
public void OnDiskToc_IsSortedAscendingByKey_RegardlessOfAddOrder() {
|
||||
var path = NewTempPakPath();
|
||||
// Add blobs in DESCENDING key order — the on-disk TOC must still come
|
||||
// out ascending (the reader's binary-search precondition, asserted
|
||||
// here against the raw bytes, not through the reader).
|
||||
var blobs = MakeBlobSet(12).OrderByDescending(b => PakKey.Compose(b.Type, b.FileId)).ToArray();
|
||||
WritePak(path, blobs);
|
||||
|
||||
var fileBytes = File.ReadAllBytes(path);
|
||||
var header = PakHeader.ReadFrom((ReadOnlySpan<byte>)fileBytes);
|
||||
Assert.Equal((uint)blobs.Length, header.TocCount);
|
||||
|
||||
ulong previousKey = 0;
|
||||
for (uint i = 0; i < header.TocCount; i++) {
|
||||
int pos = checked((int)((long)header.TocOffset + i * PakTocEntry.Size));
|
||||
var entry = PakTocEntry.ReadFrom(fileBytes.AsSpan(pos, PakTocEntry.Size));
|
||||
Assert.True(entry.Key > previousKey || i == 0,
|
||||
$"TOC entry {i} key 0x{entry.Key:X16} is not strictly greater than its predecessor 0x{previousKey:X16}");
|
||||
previousKey = entry.Key;
|
||||
}
|
||||
}
|
||||
|
||||
// ---- corruption logged once (review finding 8) ----------------------------
|
||||
|
||||
[Fact]
|
||||
public void CorruptBlob_RepeatedReads_LogExactlyOnce() {
|
||||
var path = NewTempPakPath();
|
||||
var blobs = MakeBlobSet(2);
|
||||
WritePak(path, blobs);
|
||||
|
||||
// Flip a byte in the first blob (plain CRC corruption).
|
||||
using (var fs = new FileStream(path, FileMode.Open, FileAccess.ReadWrite)) {
|
||||
fs.Position = PakHeader.Size + 4;
|
||||
int b = fs.ReadByte();
|
||||
fs.Position = PakHeader.Size + 4;
|
||||
fs.WriteByte((byte)(b ^ 0xFF));
|
||||
}
|
||||
|
||||
var victimKey = PakKey.Compose(blobs[0].Type, blobs[0].FileId);
|
||||
var originalError = Console.Error;
|
||||
var capture = new StringWriter();
|
||||
try {
|
||||
Console.SetError(capture);
|
||||
using var reader = new PakReader(path);
|
||||
// Hammer the corrupt entry through BOTH public paths, repeatedly.
|
||||
Assert.False(reader.TryReadObjectMeshData(victimKey, out _));
|
||||
Assert.False(reader.TryReadObjectMeshData(victimKey, out _));
|
||||
Assert.False(reader.ContainsKey(victimKey));
|
||||
Assert.False(reader.ContainsKey(victimKey));
|
||||
Assert.False(reader.TryReadObjectMeshData(victimKey, out _));
|
||||
}
|
||||
finally {
|
||||
Console.SetError(originalError);
|
||||
}
|
||||
|
||||
string logged = capture.ToString();
|
||||
int occurrences = CountOccurrences(logged, $"0x{victimKey:X16}");
|
||||
Assert.True(occurrences == 1,
|
||||
$"expected exactly ONE [pak-corrupt] line for key 0x{victimKey:X16} across 5 reads, got {occurrences}:\n{logged}");
|
||||
}
|
||||
|
||||
private static int CountOccurrences(string haystack, string needle) {
|
||||
int count = 0, index = 0;
|
||||
while ((index = haystack.IndexOf(needle, index, StringComparison.Ordinal)) >= 0) {
|
||||
count++;
|
||||
index += needle.Length;
|
||||
}
|
||||
return count;
|
||||
}
|
||||
|
||||
// ---- helpers -------------------------------------------------------------
|
||||
|
||||
/// <summary>Locates the on-disk file position of the TOC entry for <paramref name="key"/> by raw parsing.</summary>
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue