Add a bunch of unit tests & run on CI all the time. - #1423
Merged
Conversation
Neither accessor on `TestData` worked, and nothing consumed them closely enough to notice. - `CrossRef_File_Episode` read the `AniDB_Anime.json` resource, so it deserialised anime rows into cross-references and handed back records whose every field was left at its default - `AniDB_Anime` threw outright, because `AniDB_Anime.AirDate` is a `PartialDateOnly` and the stored `"2024-07-03 00:00:00"` has no default Newtonsoft conversion. `Shoko.Benchmarks` is the only consumer, which is why this stayed hidden Moved `PartialDateOnlyConverter` into `Shoko.TestData` so the fixtures and `FilterTests` share one copy, and added tests over both accessors.
…input Added 102 tests over logic that had none, all of them free of a database, a DI container and the network. - `AutoAnimeGroupCalculator` — the relation-graph walk and fuzzy title metric that decide how a collection is carved into groups. Covers both main-anime strategies, every `AutoGroupExclude` flag, transitive and cyclic graphs, and the title normalisation, including that `the movie` and `the animation` are stripped before comparison and that `-` splits words where other punctuation does not - `PocoCache`/`PocoIndex` — the substrate behind every cached repository. Covers index maintenance across add, update, remove and clear, and the many-valued index - `AnimeSeriesService.EpisodeList` — the part matching that decides when a split OVA or movie counts as held - `ModelHelper.GetEpisodeNumberAndTypeFromInput` — the `S3`/`C1` prefixes the v3 API accepts in range parameters - `AnimeEpisode.DefaultTitle` — the English fallback and its placeholder Added `Shoko.Tests/Infrastructure/` to reach code that reads `RepoFactory` without standing up a database. `CachedRepo` seeds a real repository's `PocoCache` and runs its own `PopulateIndexes`, so reads execute exactly as in production, and `RepoFactoryScope` installs those repositories into the `RepoFactory` statics and restores them afterwards. Those statics are process-global, so such tests join a non-parallel collection. Nothing here touches the write-once `ISystemService.StaticServices`. Also dropped `Startup` and `TestServerSettings`, both unreferenced — `Startup` is the `Xunit.DependencyInjection` convention class and that package is long gone.
`integration-tests.yml` was the only workflow invoking `dotnet test`, and it runs `Shoko.IntegrationTests` alone. `Shoko.Tests` and `Shoko.QueueProcessor.Tests` had never gated a merge, so either could be broken without CI noticing. Both suites are self-contained — no database, no network and no native dependencies — so the job needs none of the setup the integration tests do and finishes in seconds.
Two areas that had no coverage and fail quietly when they break. **Stored filter expressions.** `FilterPreset.Expression` and `SortingExpression` are persisted as JSON that records each node by its *simple* class name. `SimpleNameSerializationBinder.BindToType` resolves that name by scanning loaded assemblies and taking the first match, and returns `null` when nothing matches — which `FilterExpressionConverter` swallows through its error handler. So renaming or moving an expression turns every filter using it into a broken one, with nothing raised at the call site, and two types sharing a simple name would bind to whichever the scan happened to find first. Covers all 284 concrete expression types: each binds back to its own type, no two share a simple name, and every constructible one survives a round-trip. Also pins that a name outside the `FilterExpression` hierarchy does not bind, which is what stops stored JSON naming arbitrary types. **Sorting selectors.** 76 of them, previously untested. Beyond evaluating to a non-null comparable, each is checked against the flags the filtering engine caches on: a selector that reads user info must declare `UserDependent`, and one whose value moves with the clock must declare `TimeDependent`. `FilterableFactory` populates the filterable doubles by reflection rather than by hand, so a member added to the interface later cannot silently arrive as null in tests that look like they cover it. `TestFilterable`'s two image-type properties gained `init` so they can be populated like every sibling.
…Helpers` `FileSystemHelpers` was a concrete class, so nothing that moves or deletes a user's files could be exercised without a real disk. Extracted the interface it already implicitly had and switched every consumer to it. The concrete type stays registered and the interface resolves to the same singleton, so behaviour is unchanged. Added tests over the guards in `DirectlyRelocateFile`, which had none. They run against a mocked file system, and a rejected request is proven by the mock never being asked to move, delete or create anything: - requests with no managed folder, no relative path, or already cancelled - relative paths climbing out of the managed folder - files in an excluded folder, or in a drop destination when relocating inside destinations is disabled - a source file that is missing on disk One of these pins a subtle protection: containment is a prefix comparison, and it is only correct because `ShokoManagedFolder.Path` always ends in a directory separator. Without it a relative path could reach a sibling folder sharing a prefix — `/media/animeX` against `/media/anime` — and write outside the managed folder. Both that and the separator itself are now covered.
`UserDataService` decides whether a file counts as watched, how far through it the user is, and how many times they have seen it — the state the whole watched/unwatched view of a collection is built on — and none of it was tested. Covers marking watched and unwatched, the playback counter, progress handling including the 97.5% "near enough finished" threshold, the save/no-save decision, and the saved event. Two of these pin behaviour that is easy to regress and invisible when it goes wrong: - A watched date handed in as UTC is stored as local. The comparison against the stored date is by value, so a UTC date would otherwise read as a change on every write. Asserted on the `DateTimeKind` as well as the value, because the value alone proves nothing when the tests run on a machine set to UTC — which is what happens in CI. - An update carrying no changes must not write. Playback progress reports arrive continuously, so a needless save there is a write per second per client. `CachedRepo.BuildWritable` extends the harness to writes: a partial mock keeps every real read path and replaces only the virtual `Save`/`Delete`, landing them in the same cache the reads come from, so a saved entity is visible to the next lookup as it would be in production. It also installs a `VideoLocal` in `RepoFactory` because `VideoLocal_User.ToString()` resolves through it, and Moq calls that when rendering a failed verification.
Seven tables have no primary key on SQL Server while both SQLite and MySQL declare one: - `AniDB_Anime_PreferredImage` - `AniDB_Episode_PreferredImage` - `AniDB_FileUpdate` - `AuthTokens` - `ShokoImage_Entity` - `TMDB_Image` - `TMDB_Image_Entity` All seven were created before the version 180 sweep that added the other 38 but were missed by it. Each keys off an `IDENTITY` column, so the values are already unique and non-null and the constraint cannot fail against existing data.
Every backend carries its own copy of the schema as an ordered list of raw SQL, so the three can drift apart with nothing failing until a user on that backend hits it. Both `add missing primary keys on SQL Server for 38 tables` and `widen TMDB_Show/TMDB_Movie Genres column on MySQL and SQL Server` were that, and neither is reachable from a test that only exercises SQLite. Replays each backend's statements into a logical schema — tracking creates, drops and renames — and asserts every surviving table declares a primary key. This is what found the seven tables fixed in the previous commit. Deliberately narrow. A table-set comparison across backends was tried and dropped: distinguishing a genuine divergence from a rename this crude parser mishandles needs a real SQL parser, and a check that cries wolf is worse than no check. The primary-key assertion needs only the table name and whether the statement declares a key, which is reliable across all three dialects. `StubSettingsProvider` exists because the MySQL backend reads the settings singleton while initialising its DDL fields, so one has to be installed before it can be constructed at all.
**Automatic deletion.** `ReleaseAutoManagementService.ComputeRedundantPlaces` decides which of a user's files get removed when a better release of the same episodes is present. It is the most destructive decision the server makes unattended and it had no tests. It returns the list rather than acting on it, so every rule is checkable without deleting anything, and the real `ReleaseComparisonService` is used rather than a stub so the actual ranking runs. The two that matter most: - A file belonging to the primary is never deleted, even when the same physical file also appears in a secondary gap-fill candidate. Removing the guard that enforces this makes both of those tests fail. - The eligibility gate holds. A primary that is mixed, corrupted, or has no release info cannot cause deletions, and the bypass is reserved for a primary the user picked by hand. Also covers per-file versus whole-candidate mode for airing series, and that a file whose episode coverage cannot be resolved is always kept. **Persistence converters.** The `IUserType` implementations sit between entity properties and columns, so one that loses information corrupts data silently. Checks the contract every converter owes NHibernate across all twelve — column types, returned type, and that two nulls compare equal, which dirty-checking runs on every flush — plus round-trips for MessagePack, string lists, partial and whole dates, types and JSON. One pins a limitation rather than a guarantee: `StringListConverter` joins on `"|||"` with no escaping, so an entry containing that separator comes back as two entries. Better recorded here than discovered in someone's data.
`AnimeSeriesService.UpdateStats` computes the counts behind the missing-episode filters, the dashboard and the calendar, and nothing else recomputes them — a series carries whatever this last wrote. Covers episodes with and without files, that un-aired episodes are not counted as missing, that hidden episodes are counted separately, that only regular episodes count (not specials or credits), that the counts are recomputed rather than accumulated across runs, and the derived `LatestLocalEpisodeNumber` and `LatestEpisodeAirDate`. Marked `AnimeSeriesRepository.Save(AnimeSeries, bool, bool)` and `AniDB_GroupStatusRepository.GetByAnimeID` as `virtual` so they can be stood in for. `virtual` alone changes no behaviour, and it follows what was already done ad hoc for the handful of methods an earlier test needed.
`AniDBUDPConnectionHandler` constructed its `AniDBSocketHandler` inline, so none of the protocol handling around the socket could be reached without binding a port. `IAniDBSocketHandler` already existed; this adds the factory to go with it and switches the field to the interface. The real factory returns the same socket handler as before, so behaviour is unchanged.
Neither connection handler was tested, and both carry the logic that keeps the server out of an AniDB ban — which costs a user a day of metadata when it goes wrong. **HTTP.** A successful call returns the body and status; a response containing the `>banned<` marker sets the ban and throws; a further call while banned never reaches the network, because talking to AniDB while banned is what extends the ban; `force` still gets through for the caller that needs it; and a server error or transport failure is surfaced rather than mistaken for a ban. **UDP.** `Init` builds the socket and records whether it connected, and refuses incomplete credentials. Commands go out as UTF-16 or ASCII as asked, replies are decoded by their byte order mark and the mark is stripped. An all-zero reply is treated as a ban — a silent socket cannot be told apart from one, and assuming the worse is what stops the server digging deeper — after which `Send` stops talking. **No test opens a connection of any kind.** HTTP goes through a stub `HttpMessageHandler` that answers from a queue, and UDP through a stub `IAniDBSocketHandler` that replays canned payloads. Both are pointed at `anidb.invalid` — reserved by RFC 2606 and guaranteed never to resolve — so anything that did try to reach the network would fail loudly rather than quietly succeed.
`HttpAnimeParser` turns AniDB's XML into the anime, title and episode records the rest of the server is built on. It is pure — XML in, objects out — and had no tests, in one of the most frequently fixed areas of the codebase. Covers the documents it must reject (no anime id, no main title), the anime type mapping, episode counts, the restricted flag, titles with their type and language, and the episode fields including type prefixes, double episodes, and length in minutes becoming seconds. Two pin AniDB quirks that are invisible until they break: - `1970-01-01` is AniDB's "unknown date" sentinel. Taking it literally would file shows under 1970 and break every year filter and season grouping. - Apostrophes arrive as backticks throughout the API and are translated back in titles and descriptions.
`StringListConverter`, `TmdbContentRatingConverter` and `TmdbProductionCountryConverter` all return a `List<T>`, and all compared with `x.Equals(y)`, which for a list is reference equality. `NullSafeGet` builds a fresh list on every load, so the loaded value never matched the stored one and NHibernate treated every mapped list column as changed on every flush, rewriting rows that nobody had touched. The columns affected are the AniDB title/tag lists and the TMDB content ratings and production countries. They now compare the string each side would be written as, so two lists are the same exactly when they would produce the same column value.
…fail An adversarial review of the new suites found several that passed regardless of what the production code did. Verified by mutation, before and after. **Sorting selectors.** 74 of the 75 are a single property read, so "produces a value", "is comparable", "is deterministic" and "ignores the time" could never be false; and the two value assertions could not tell one property from another, because `FilterableFactory` gave every property of the same type the same sample. Changing a selector to return a sibling field kept all 381 cases green. The factory now seeds each value from the property name, and the selectors are pinned by a generated table of selector to the field it reads. Four selectors mutated to read a sibling — plain, nested, user-scoped, and the date-converting one — now each fail. **Converters.** Both equality theories passed `x` as both arguments or two nulls, so only the `ReferenceEquals` short-circuit ran and the comparison that decides whether a row is rewritten was never reached. Replaced with distinct-but-equal values per converter, which caught the list-comparison bug fixed in the previous commit. Dropped the two that asserted a hardcoded `typeof()` was not null and one that asserted nothing at all, and added `MessagePackConverter<MediaContainer>` explicitly, since reflection never yields an open generic and it had been excluded from every theory. **AniDB HTTP.** `TheRequestGoesToTheConfiguredServer` only checked that the relative URL it passed in came back, so the handler could ignore the configured server entirely and stay green. Asserts the whole URI now. Also: assert the actual date rather than not-null in the series stats, tighten an over-broad `ThrowsAny`, drop a strictly weaker duplicate in the release management tests, and correct a class comment that claimed ranking was exercised when only redundancy is.
The file was named for a cross-backend comparison but never made one — every assertion ran against a single backend, so deleting a table from MySQL alone kept it green. Two replay bugs had to be fixed first, or real drift would have been indistinguishable from parser noise: - A table rename now requires `RENAME TO`. The old pattern also matched `ALTER TABLE x RENAME <col> TO <col>`, a column rename, which renamed the table to the column name and left a phantom table behind. - Command strings holding several `;`-separated statements are now split. Only the first was ever examined, so a create or drop following a rename in the same migration was invisible. With those fixed the backends agree on every table but `Language`, which all three drop — SQLite from a coded migration that a static replay cannot observe, so it is excluded and the reason recorded. Removing one `CREATE TABLE` from MySQL alone now fails.
`GetEpisodeNumberAndTypeFromInput` reads `input[0]` with no length check, so an empty string throws `IndexOutOfRangeException` instead of returning the error tuple every other bad input gets. It is reachable from the v3 range parameters, where it surfaces as a 500 rather than a validation message. Written against the intended behaviour and skipped with "Possible bug - Needs investigation", so it documents what should happen and turns green when the guard is added, rather than asserting the current behaviour and enshrining it. Verified that adding the guard makes it pass.
The review found three paths in `UpdateMissingEpisodeStats` that no test could reach, two of them because the harness could not express the input. - Un-aired episodes were arranged with `AirDate = 0`, which AniDB means as *no date*, not *the future*. That took the fallback to whether the series had finished, and passed only because the harness left the anime's end date unset. A spec can now carry a real air date, so the future-dated comparison is exercised, and the unknown-date fallback is covered separately in both directions. - The group-status list was always stubbed empty, which forces every aired episode to count as released and leaves `MissingEpisodeCountGroups` structurally zero. A spec can now attach a release group to a file, so the "released by a group I collect" total is covered — along with an episode no group has released yet, which should not count as missing at all. Setting `epReleased` to true, or `epReleasedGroup` to false, now each fail a test. Two things this shook out about the setup rather than the code: a release only exposes its group once all four group fields are set, and two harnesses must not be alive at once, since both install into the same `RepoFactory` statics.
Seeding each value from its property name left one hole: every collection was built with exactly one element, so every `.Count` read returned 1 and the four selectors reading a collection count — audio languages, subtitle languages, custom tags, user tags — were indistinguishable from each other. Swapping any of them to a sibling collection kept the suite green. Collections are now sized from the name as well as filled from it, and a guard asserts that two selectors reading different fields never resolve to the same value on the double. That is the property the whole table depends on, so it is checked rather than assumed: returning every set to a single element now fails the guard, and the two count selectors above now fail when swapped. Selectors that genuinely read the same field are allowed to agree — there are two, both reading `SortName`.
…ation The test asserts the right result but fails roughly one full run in ten, and never when its class runs alone — so it would have flaked CI and nowhere else. The cause is not the test. `UpdateMissingEpisodeStats` updates `latestLocalEpNumber` and `lastEpAirDate` from inside an `AsParallel` body without synchronising either, while every neighbouring accumulator in the same lambda is explicitly locked. A lost update leaves a stale value on the series, which drives continue-watching ordering and the calendar. Skipped rather than fixed in place, and recorded for investigation. Guarding one of the two accumulators was tried and did not settle it, so the extent is not yet established.
**Converter equality had no negative direction.** Every assertion was `Assert.True`, so replacing any converter's `Equals` with `=> true` passed the whole suite — the mirror of the bug fixed in `999db949d`, and worse, since NHibernate would then never write a changed column back at all. Added the unequal-value direction, which now fails for both converters when their comparison is stubbed out. The table also skipped the converters that inherit whatever equality their entity type happens to have, including the two heaviest columns in the schema. `MessagePackConverter<MediaContainer>` had been added to discovery with a comment about having been missed, then left out of the one test the comment was about. Covered now, and the TMDB rows use non-empty lists, which the empty ones could never have distinguished. **The `Language` exclusion was unconditional**, so it also hid MySQL or SQL Server failing to drop the table. Only SQLite migrates it from code, so only SQLite is excluded; stubbing out MySQL's `DROP TABLE` now fails. **Nothing checked the selector table covered every selector** — one was silently absent, and anything added later would have been covered only by "returns something comparable". Also: scalars and collection sizes now occupy disjoint ranges, after the collision guard caught a `.Count` matching a scalar and then two scalars matching each other; and the schema remarks no longer claim to catch a column-width regression, which nothing there models.
The playlist DSL is user-supplied text from the v3 API, so how it turns bad input away matters as much as what it accepts. Covers unknown and non-positive group IDs, non-positive release group IDs, and entries carrying more than a group and a release group. Writing these turned up that the documented `g<id>+<extra>` suffix cannot be parsed at all. Entries are split on `+` before the extras are looked for, so the suffix is never found and survives as a separate sub-item, which is then rejected. `recursive` fares worst: it begins with `r`, so it is taken for a release group ID. None of the seven documented flags can be used. Recorded rather than fixed, with the four documented forms as a skipped theory. Only the rejection paths are asserted here — once an entry parses the service builds the playlist, which needs the full service graph.
**An empty playlist entry was not really covered.** The test asserted only that no error was raised, which stays true with the guard it was named for deleted — an entry producing nothing is discarded further down anyway. The skip is not observable from outside; what is observable is that the entry still consumes a position, so the assertion is now on the error being attributed to the right index. **Four converters looked covered for equality but were not.** They appeared only among the equal values, where `Assert.True(Equals(a, b))` is satisfied by a stuck-true implementation — the very failure the pair of theories exists to catch. Covered in both directions now, and a check asserts the two tables stay paired, so a converter cannot be added to one and forgotten in the other. Also corrected the diagnosis recorded for the playlist extras bug. The split consuming the `+` is only half of it: `IndexOf(['+', ' '])` binds to the span overload and searches for the two-character sequence `"+ "`, which never occurs. Either correction alone still fails; with both, the parser accepts all four documented forms, which is now measured rather than assumed.
… fix Replaces the earlier placeholder with the six examples taken from `ParsePlaylist`'s own remarks, covering the group and series branches, and written against the intended behaviour so it turns green when the parsing is corrected. The entry is split on `+` before the extras suffix is looked for, so the suffix survives as a separate sub-item and is rejected; `recursive` is taken for a release group ID because it begins with `r`. This has never worked — the first version split on `+` and then searched the result for `+`, which could never match — so nothing regressed, and two years without a report suggests the flags are unused. Correcting the split alone is not enough: `IndexOf(['+', ' '])` binds to the span overload and looks for the literal sequence `"+ "`, so `IndexOfAny` is needed too. Both were measured.
The documented `g<id>+<extra>` syntax cannot be demonstrated as a defect. `items` arrives through `[FromQuery]`, and a query string decodes `+` to a space, so a client following the documentation sends `g5+recursive` and the service receives `g5 recursive`. The literal `+` only survives if the client percent-encodes it. That makes the suffix indistinguishable from a second sub-item once decoded, which is why the parser accepts either delimiter, and means the syntax is not expressible over this transport rather than being mis-parsed. The remaining rejection tests are unaffected.
`Install()` was an unsynchronised check-then-act against the process-global `ISettingsProvider.Instance`. `AniDBHttpConnectionTests` writes `HTTPServerUrl` onto whatever provider is installed after calling it, and `SchemaParityTests` installs from a different, parallelisable collection — so both could see the static unset, both install, and the loser's settings be discarded. Also tidied stray whitespace in `PocoCacheTests` and a missing trailing newline in `PlaylistParsingTests`.
`999db949d` made `IUserType.Equals` value-based on `StringListConverter`, `TmdbContentRatingConverter` and `TmdbProductionCountryConverter`, but left `GetHashCode` hashing the `List<T>` reference. Two values NHibernate now considers equal therefore hashed differently, which breaks the contract its user-type caching relies on. Both sides now key off the serialized form, so equal values hash equally.
It had a `Release|Any CPU.ActiveCfg` but no matching `Build.0`, so a solution build in Release skipped it while still resolving references to it. Debug built fine, which is why nothing had noticed; a Release build of anything referencing it failed with `CS0234`.
The three backends each keep their own hand-written DDL, and nothing forced them to agree. `SchemaParityTests` compared tables and primary keys only, so a column added, widened or made nullable on one backend and missed on another went unnoticed — `add missing primary keys on SQL Server for 38 tables` and `widen TMDB_Show/TMDB_Movie Genres column on MySQL and SQL Server` were both that. The new comparison reads the catalog of a real database of each backend rather than replaying the DDL, because a replay cannot see the whole migration: MySQL performs some of its through `PREPARE stmt FROM @sqlstmt`, and every backend has migrations written in C#. Nothing is committed — a recorded schema is a copy that falls behind the migrations it claims to describe — so the dumps are produced at runtime and compared side by side. - `SchemaSnapshot` reduces a backend's catalog to a type family, declared width and nullability per column, so the dialects can be compared at all - `SchemaSnapshotTests` writes the dump for the backend it just migrated - `SchemaTypeParityTests` compares the three, and skips rather than passes when they are not all present - CI publishes each job's dump and adds a `schema-parity` job over all three - `scripts/compare_schemas.sh` runs the whole thing locally against Docker SQLite takes no part in the width comparison, having no declared widths, and its `INTEGER` is treated as `BIGINT`, which it is. The comparison currently fails: 6 columns differ in type, 15 in width between MySQL and SQL Server, and 110 in nullability. It reports them in full so the list can be worked through.
Both test classes took `DatabaseMigrationFixture` as a class fixture, which is one instance per class, so the second bootstrap hit the write-once `ISystemService.StaticServices` and failed with `The service provider has already been set`. Running either class alone passed, which is why it was not caught when `SchemaSnapshotTests` was added. Moved to a collection fixture, so the whole run shares one.
`SchemaTypeParityTests` found 110 columns whose nullability differed between the three backends. Every one of them backs a non-nullable model property — `string MainTitle`, `DateTime LastUpdatedAt` — so a null could never have been read back into one, and SQLite already agreed with the model on 93 of the 95 it could be checked against. Most of the MySQL half was one cause: `MySQLFixUTF8` and `MySQLFixUTF8MB4` rebuild every text column with `MODIFY`, which replaces the whole column definition and silently drops any attribute left unstated. 83 columns lost `NOT NULL` that way, `AniDB_Anime.MainTitle` among them, despite its `CREATE TABLE` declaring it. Those two are versioned migrations that have already run, so they are left alone and the columns are repaired instead. - MySQL v185 tightens 90 columns - SQL Server v183 tightens 25, mostly TMDB `CreatedAt`/`LastUpdatedAt` - SQLite v164 tightens `AniDB_Anime_Title.Title` and `VideoLocal.DateTimeCreated` Each fills its nulls before altering, since a stored null would fail the alter. SQLite cannot tighten a column in place, so `MakeColumnNotNull` rebuilds the table around a `CREATE TABLE` patched from the one the database reports. It has to come from the database: `MoveAnidbFileDataToReleaseInfoFormat` is a `PostDatabaseFix`, which runs after every other command, so a database migrating in one pass still has the `CRC32`, `MD5` and `SHA1` columns that one migrating from an older version dropped long ago. Both shapes are covered by `SqliteNotNullVariantTests`. Verified against all three backends, migrating from empty and, for MySQL and SQL Server, upgrading a database already at the previous version.
…ated The v185 list was derived from a database Shoko had created itself, which uses utf8mb4_unicode_ci — so `MySQLFixUTF8MB4` had nothing to convert for six columns and they kept their `NOT NULL`. A database created outside Shoko keeps the server's default collation instead, and the conversion then reaches those too. CI creates the schema through `MARIADB_DATABASE`, so it hit exactly that and reported six divergences the local run could not: `AniDB_Episode.Rating`, `AniDB_Episode.Votes`, `AnimeEpisode_User.UserTags`, `AnimeSeries_User.UserTags`, `Versions.VersionType` and `Versions.VersionValue`. Since every text column is converted when none of them match, this is the whole of the damage rather than another instalment of it. Verified against a database created the way CI creates it, both migrating from empty and upgrading one already at v185.
`BindToType` scans every loaded assembly, and `GetTypes()` throws `ReflectionTypeLoadException` on one that is still being written to. The throw reaches `FilterExpressionConverter.ConvertFrom`, whose error handler sets `Handled` and hands back `null`, so the filter comes back blank rather than failing. CI hit it on six of the 287 `EveryConstructibleExpressionSurvivesARoundTrip` cases, spread across unrelated namespaces — whichever happened to be deserializing while a Castle proxy was being emitted for a mock elsewhere in the run. Nothing bound here is ever emitted at runtime, so skipping dynamic assemblies costs nothing. Reproduced by mocking distinct interfaces while scanning, which threw 32, 52 and 40 times across three runs, and threw none once the assemblies were skipped.
…t racing timers `SimpleNameSerializationBinderTests` reproduces the scan failing while assemblies are emitted alongside it; it fails on all three runs without the fix in the previous commit. `SlotsExpireAfterWindow_AllowsNewCalls` asserted that two calls finish within 200ms after a 200ms window expires. `SegmentsPerWindow` is 10, so slots came back in 20ms slices — finer than a loaded runner services its timers, and it measured 373ms in CI. Widened to a 1000ms window so a slice is 100ms, with proportionally more slack.
`212e9a7d3` fixed one of these. `TypeStringConverter` had the same scan and failed the same way — `ReflectionTypeLoadException: Could not load type 'Castle.Proxies.ITextStreamProxy' from assembly 'DynamicProxyGenAssembly2'` — so the fix belongs in one place rather than wherever the next failure happens to land. Thirteen scans looked up job types, subtitle providers, filter expressions, AniDB request types and mapped entities through `AppDomain.CurrentDomain.GetAssemblies()`. None of those are ever emitted at runtime, so none of them need the assemblies that are, and `Assembly.GetTypes()` throws on one that is still being written to. `ReflectionUtils.ScannableAssemblies` now states that once. `Shoko.QueueProcessor` cannot reference it, so its one scan says the same thing locally.
The suite measured how long things took in seven places, which fails on a runner under load and says nothing about the code. Two did fail: the rate limiter measured 1933ms against a 500ms bound, and a `Speed Test` averaged 2911ms against a 600ms target. - `TmdbRateLimiterTests` now asserts what the limiter reports — `RemainingInWindow`, `CallsInWindow` and `BackoffUntilTicks`. That a caller with no permit left is made to wait is `SlidingWindowRateLimiter`'s job; ours is to hand it the right window and record the backoff, which is what those show. Mutation-checked: reporting full capacity, and never recording a deadline, each fail two of them. - `TagFilterTest.TestSpeed` asserted an average under 2000ms and nothing else. That is a benchmark, so it moved to `Shoko.Benchmarks` as `TagFilterBenchmarks`. - `ReflectionUtilsTests` covers the shared assembly scan; removing the guard fails it every time, where the flake it replaces showed up about once in ten full runs. No assertion on elapsed time is left in any of the three test projects.
…them `TheScanSurvivesAssembliesBeingEmittedAlongsideIt` passed its cancellation token to `Task.Run`. A task that has not been scheduled by the time the token fires is returned cancelled rather than run, so `Task.WhenAll` threw `TaskCanceledException` on a runner with fewer cores than the eight tasks it starts. The token now only ends the loops. Confirmed by firing it immediately: cancelled with the token passed, clean without it. `OnEnqueue_MaxBatchReached_TriggersImmediateFlush` slept 100ms and asserted a fire-and-forget flush had happened. It now waits to be told, with a timeout well clear of any scheduling delay. Still fails, as a timeout, when the force flush is disabled.
…ready uses `upload-artifact@v4` and `download-artifact@v5` still run on Node 20, which GitHub now warns about. Every other workflow here is already on `upload-artifact@v7` and `download-artifact@v8`.
…does v170 made twelve columns `utf8mb4_bin` so that hashes, paths and tokens compare case-sensitively. v185 rebuilt ten of them with `MODIFY`, which replaces the whole definition, and every one of those named `utf8mb4_unicode_ci` — quietly making `VideoLocal.Hash`, `VideoLocal_Place.FilePath` and `ImportFolder.ImportFolderLocation` case-insensitive again on any database upgrading through it. Each of the ten now restates the collation the column already has. `MySQLFixUTF8` undid the same thing, and had been doing so since v170. It is a `PostDatabaseFix`, so on a database migrating in one pass it runs after every patch, and it converted anything not already `utf8mb4_unicode_ci` — v170's columns included, dropping their `NOT NULL` along the way. A fresh install therefore ended up case-insensitive where one that upgraded step by step, having recorded the fix long ago, stayed case-sensitive. It now leaves `utf8mb4_bin` alone, which is already utf8mb4 and never what it was written to convert. `SchemaTypeParityTests` cannot see a collation, so neither showed up there. Verified on MariaDB migrating from empty: all twelve keep `utf8mb4_bin`, and no column is left on any other collation.
`SchemaTypeParityTests` found six columns SQLite stores under a different type family than MySQL and SQL Server do: - `AniDB_Anime.AirDate` and `EndDate` became `varchar(10)` on MySQL at v171 and on SQL Server at v167, when they became a `PartialDateOnly`. SQLite never got that migration and kept the `DATETIME` it was created as, so rows written before the change still carry a time of day the other two dropped - `AnimeEpisode_User.UserTags` and `AnimeSeries_User.UserTags` were added by an `ALTER TABLE ... ADD COLUMN` that named no type at all, leaving them with BLOB affinity where the model maps a `StringListConverter` - `TMDB_Episode.Runtime` and `TMDB_Movie.Runtime` map `RuntimeMinutes`, an `int?`, but were created `TEXT` SQLite cannot retype a column in place, so v165 rebuilds each table the way v164 does for nullability. `RetypedVariantOf` patches the type into the table's own `CREATE TABLE`, read from the database rather than written out here, and replaces only the type: the constraints that follow carry the nullability, the default and the primary key. It has to cope with a type that is several words, brackets a comma, or is not there at all, all of which `SqliteRetypedVariantTests` covers. Verified against all three backends migrating from empty.
`SchemaTypeParityTests` found fifteen columns MySQL and SQL Server declare at different widths. In every case one of the two already bounds the column and the other leaves it unbounded, so the bound is what the data already is — nothing longer can ever have been written on the backend that enforces it. Each takes the bound. - MySQL v186 narrows eight `text` columns SQL Server bounds: `AniDB_Anime_Relation.RelationType` to 100, `ScanFile.Hash` and `HashResult` to 100, `AnimeSeries.AirsOn` to 10, `FilterPreset.Name` to 250, and three user-entered titles to 500 - SQL Server v184 narrows seven `MAX` columns MySQL bounds: `AniDB_Episode.Rating` and `Votes` to 200, `AnimeSeries`'s two default languages to 50, both `ImportFolder` columns to 500, and `TMDB_Person.PlaceOfBirth` to 128 Each trims its values to the width first, since anything longer would fail the alter. `FilterPreset.Name` drops its index before the alter and recreates it after: MySQL indexed it by a 255-character prefix, which no longer fits a 250-character column, and a bounded column does not need one. Verified against all three backends, migrating from empty and, for MySQL and SQL Server, upgrading a database seeded with values too long for the new width — every one is trimmed rather than failing the migration.
`compare_schemas.sh` created the MariaDB database with an explicit `utf8mb4_unicode_ci`, which is what Shoko's own `CREATE DATABASE` uses. CI creates it through `MARIADB_DATABASE`, which keeps the server default, and so does anyone who creates one outside Shoko — and only then does `MySQLFixUTF8MB4` reach the columns it leaves alone otherwise. The local run therefore could not reproduce what CI reported. It now names no collation either.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.