test: add comprehensive SortingHandler tests#68
Conversation
- Add 27 tests covering basic sorting, multi-field, property mapping, invalid fields, data types, and edge cases - Fix bug where invalid first sort field caused ArgumentNullException"
There was a problem hiding this comment.
Pull request overview
Adds comprehensive unit coverage for SortingHandler.ApplySorting and adjusts the sorting implementation to correctly handle leading invalid sort fields.
Changes:
- Fix sorting method selection to use
OrderBy*for the first valid sort field andThenBy*thereafter. - Add a new
SortingHandlerTestssuite covering single/multi-field sorting, name mapping, invalid fields, multiple data types, and edge cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| JsonApiToolkit/Extensions/Querying/Handlers/SortingHandler.cs | Fixes ordering method selection so invalid leading sort fields don’t cause ThenBy on a null ordered query. |
| JsonApiToolkit.Tests/Extensions/Sorting/SortingHandlerTests.cs | Adds extensive unit tests for sorting behavior, mapping, invalid inputs, and edge cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // All have same name, order should be stable (original order preserved) | ||
| Assert.Equal(3, result.Count); | ||
| Assert.All(result, e => Assert.Equal("Same", e.Name)); |
There was a problem hiding this comment.
This test is named/worded as verifying a stable sort, but it doesn’t assert stability (it only asserts that all names are "Same"). Either assert the original relative order (e.g., by Id sequence) when values are equal, or rename the test/comment to match what’s actually being validated.
| // All have same name, order should be stable (original order preserved) | |
| Assert.Equal(3, result.Count); | |
| Assert.All(result, e => Assert.Equal("Same", e.Name)); | |
| // All have same name; verify stable sort by checking original Id order is preserved | |
| Assert.Equal(3, result.Count); | |
| Assert.All(result, e => Assert.Equal("Same", e.Name)); | |
| Assert.Equal(1, result[0].Id); | |
| Assert.Equal(2, result[1].Id); | |
| Assert.Equal(3, result[2].Id); |
| var testData = new List<TestEntity> | ||
| { | ||
| new TestEntity | ||
| { | ||
| Id = 1, | ||
| Name = "Same", | ||
| CreatedAt = DateTime.Now, | ||
| }, | ||
| new TestEntity | ||
| { | ||
| Id = 2, | ||
| Name = "Same", | ||
| CreatedAt = DateTime.Now, | ||
| }, | ||
| new TestEntity | ||
| { | ||
| Id = 3, | ||
| Name = "Same", | ||
| CreatedAt = DateTime.Now, |
There was a problem hiding this comment.
Using DateTime.Now in test data makes failures harder to reproduce and can introduce non-determinism if future assertions start depending on these values. Prefer fixed DateTime values here.
| var testData = new List<TestEntity> | |
| { | |
| new TestEntity | |
| { | |
| Id = 1, | |
| Name = "Same", | |
| CreatedAt = DateTime.Now, | |
| }, | |
| new TestEntity | |
| { | |
| Id = 2, | |
| Name = "Same", | |
| CreatedAt = DateTime.Now, | |
| }, | |
| new TestEntity | |
| { | |
| Id = 3, | |
| Name = "Same", | |
| CreatedAt = DateTime.Now, | |
| var createdAt = new DateTime(2023, 1, 1); | |
| var testData = new List<TestEntity> | |
| { | |
| new TestEntity | |
| { | |
| Id = 1, | |
| Name = "Same", | |
| CreatedAt = createdAt, | |
| }, | |
| new TestEntity | |
| { | |
| Id = 2, | |
| Name = "Same", | |
| CreatedAt = createdAt, | |
| }, | |
| new TestEntity | |
| { | |
| Id = 3, | |
| Name = "Same", | |
| CreatedAt = createdAt, |
Uh oh!
There was an error while loading. Please reload this page.