Conversation
Contributor
Reviewer's GuideUpdates the QueryPageOptions serialization unit test to use a correctly spelled variable name and temporarily expect empty collections instead of single-element collections while keeping other field assertions intact. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The test now asserts that several collections are empty and leaves the previous
Assert.Singlechecks commented out with a "temporary" comment; before merging, either restore the original expectations or update the test to clearly reflect the intended long-term behavior without commented-out assertions. - If the underlying behavior has intentionally changed to return empty collections, consider renaming or updating the test description to clarify this new contract instead of implying a temporary workaround.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The test now asserts that several collections are empty and leaves the previous `Assert.Single` checks commented out with a "temporary" comment; before merging, either restore the original expectations or update the test to clearly reflect the intended long-term behavior without commented-out assertions.
- If the underlying behavior has intentionally changed to return empty collections, consider renaming or updating the test description to clarify this new contract instead of implying a temporary workaround.
## Individual Comments
### Comment 1
<location> `test/UnitTest/Extensions/QueryPageOptionsExtensionsTest.cs:193-197` </location>
<code_context>
+ Assert.True(expected.IsVirtualScroll);
+ Assert.NotNull(expected.SearchModel);
+
+ // 临时更改为空集合
+ Assert.Empty(expected.Searches);
+ Assert.Empty(expected.AdvanceSearches);
+ Assert.Empty(expected.CustomerSearches);
+ Assert.Empty(expected.Filters);
+
+ //Assert.Single(expected.Searches);
</code_context>
<issue_to_address>
**issue (testing):** New expectations of empty collections look inconsistent with how the model is populated and may hide a regression.
Here the model is still built with searches and filters, but the assertions now expect all those collections to be empty. That either means the implementation stopped serializing them (possible regression) or the test has been relaxed to pass. Please align the test with the intended behavior: if these collections should survive JSON round‑trip, restore the `Single`/`Count` checks; if they should be cleared, stop populating them in the test setup and make that behavior explicit in the test name/comments.
</issue_to_address>
### Comment 2
<location> `test/UnitTest/Extensions/QueryPageOptionsExtensionsTest.cs:199-202` </location>
<code_context>
+ Assert.Empty(expected.CustomerSearches);
+ Assert.Empty(expected.Filters);
+
+ //Assert.Single(expected.Searches);
+ //Assert.Single(expected.AdvanceSearches);
+ //Assert.Single(expected.CustomerSearches);
+ //Assert.Single(expected.Filters);
+ Assert.Equal(2, expected.SortList.Count);
+ Assert.Equal(2, expected.AdvancedSortList.Count);
</code_context>
<issue_to_address>
**suggestion (testing):** Avoid leaving commented‑out assertions; either restore them or remove them and encode the intent in the test.
These commented `Assert.Single(...)` calls obscure what behavior the test is actually validating and will likely confuse future maintainers. If they still describe the intended behavior, re‑enable them (updating expected counts as needed). If the behavior has changed and empties are now correct, remove the legacy asserts and briefly document why these collections are expected to be empty instead of leaving outdated expectations commented out.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7317 +/- ##
==========================================
- Coverage 99.99% 99.98% -0.02%
==========================================
Files 745 746 +1
Lines 32637 32561 -76
Branches 4523 4500 -23
==========================================
- Hits 32636 32556 -80
- Misses 0 1 +1
- Partials 1 4 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the unit test for QueryPageOptions serialization/deserialization. The changes include fixing a typo in the variable name and temporarily modifying assertions to check for empty collections instead of single items, as noted by the "work in progress" (WIP) status in the title.
- Fixed spelling of variable name from
expactedtoexpected - Changed assertions to check for empty collections instead of single items (marked as temporary)
- Commented out the original assertions that validated collection contents
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+193
to
+202
| // 临时更改为空集合 | ||
| Assert.Empty(expected.Searches); | ||
| Assert.Empty(expected.AdvanceSearches); | ||
| Assert.Empty(expected.CustomerSearches); | ||
| Assert.Empty(expected.Filters); | ||
|
|
||
| //Assert.Single(expected.Searches); | ||
| //Assert.Single(expected.AdvanceSearches); | ||
| //Assert.Single(expected.CustomerSearches); | ||
| //Assert.Single(expected.Filters); |
There was a problem hiding this comment.
The comment "临时更改为空集合" (temporary change to empty collection) indicates this is a work-in-progress change. This test is now checking for empty collections instead of single items, which contradicts the test setup where items are explicitly added to these collections (lines 171-174). This makes the test ineffective as it no longer validates the serialization/deserialization of these collections.
Suggested change
| // 临时更改为空集合 | |
| Assert.Empty(expected.Searches); | |
| Assert.Empty(expected.AdvanceSearches); | |
| Assert.Empty(expected.CustomerSearches); | |
| Assert.Empty(expected.Filters); | |
| //Assert.Single(expected.Searches); | |
| //Assert.Single(expected.AdvanceSearches); | |
| //Assert.Single(expected.CustomerSearches); | |
| //Assert.Single(expected.Filters); | |
| Assert.Single(expected.Searches); | |
| Assert.Single(expected.AdvanceSearches); | |
| Assert.Single(expected.CustomerSearches); | |
| Assert.Single(expected.Filters); |
Comment on lines
+199
to
+202
| //Assert.Single(expected.Searches); | ||
| //Assert.Single(expected.AdvanceSearches); | ||
| //Assert.Single(expected.CustomerSearches); | ||
| //Assert.Single(expected.Filters); |
There was a problem hiding this comment.
These commented-out assertions should either be removed or uncommented. Leaving commented code in the codebase reduces maintainability and creates confusion about which assertions are actually being tested.
Suggested change
| //Assert.Single(expected.Searches); | |
| //Assert.Single(expected.AdvanceSearches); | |
| //Assert.Single(expected.CustomerSearches); | |
| //Assert.Single(expected.Filters); |
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
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.
Link issues
fixes #7316
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Tests: