Skip to content

Conversation

@labsin
Copy link

@labsin labsin commented May 4, 2025

Adds a ContainSubtree overload with the Func<IJsonAssertionOptions<object>, IJsonAssertionOptions<object>> config parameter analogue with the BeEquivalentTo methods.

Fixes #78

@dennisdoomen dennisdoomen requested a review from Copilot September 7, 2025 10:59
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds a new ContainSubtree overload that accepts a configuration function parameter, providing users with more control over JSON assertion options similar to the existing BeEquivalentTo methods. This enhancement addresses issue #78 by allowing custom assertion configurations when checking JSON subtrees.

  • Adds a new ContainSubtree method overload with configurable assertion options
  • Includes comprehensive test coverage for float approximation scenarios
  • Updates XML documentation parameter name for clarity

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
Src/FluentAssertions.Json/JTokenAssertions.cs Adds new ContainSubtree overload with config parameter and fixes XML doc parameter name
Tests/FluentAssertions.Json.Specs/JTokenAssertionsSpecs.cs Adds test cases for float approximation using the new ContainSubtree overload

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@dennisdoomen dennisdoomen changed the title add ContainSubtree that takes a config Add ContainSubtree that takes a config Sep 28, 2025
@dennisdoomen dennisdoomen changed the title Add ContainSubtree that takes a config Add overload of ContainSubtree that takes a config Sep 28, 2025
Copy link
Member

@dennisdoomen dennisdoomen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of minor suggestions. You do need to update the snapshot tests using the AcceptApiChanges.ps1 to make the build green.

Also, apologies for the late review.

@dennisdoomen dennisdoomen requested a review from jnyrup September 28, 2025 11:17
@labsin
Copy link
Author

labsin commented Sep 30, 2025

I also removed the bad copy-paste example doc and the misplaced end tag that the AI noticed

@labsin labsin requested a review from dennisdoomen September 30, 2025 07:04
@jnyrup
Copy link
Member

jnyrup commented Oct 2, 2025

The API update wasn't successful.
I guess you need to compile the project before running AcceptChanges.

Besides that - the changes look good.

@dennisdoomen
Copy link
Member

@labsin are you planning to address the rework anytime in the coming days?

@labsin labsin force-pushed the ContainSubtree-config-overload branch from df424ea to ff9be26 Compare October 21, 2025 06:27
@labsin labsin force-pushed the ContainSubtree-config-overload branch from ff9be26 to 061f6fd Compare October 21, 2025 06:36
@labsin
Copy link
Author

labsin commented Oct 21, 2025

I rebased on master and redid the AcceptChanges.
dotnet test now succeeds on my end

@coveralls
Copy link

Pull Request Test Coverage Report for Build 18675139399

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 91.144%

Totals Coverage Status
Change from base Build 18659314042: 0.02%
Covered Lines: 362
Relevant Lines: 384

💛 - Coveralls

@dennisdoomen dennisdoomen requested review from jnyrup and removed request for jnyrup October 21, 2025 15:10
/// json.Should().ContainSubtree(JToken.Parse("{ items: [ { type: 'my-type', name: 'Alpha' } ] }"));
/// </code>
/// </example>
public AndConstraint<JTokenAssertions> ContainSubtree(string subtree, string because = "", params object[] becauseArgs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also augment this overload with one that takes a config?

Comment on lines +503 to +517
/// <summary>
/// Recursively asserts that the current <see cref="JToken"/> contains at least the properties or elements of the specified <paramref name="subtree"/>.
/// </summary>
/// <param name="subtree">The subtree to search for</param>
/// <param name="config">The options to consider while asserting values</param>
/// <param name="because">
/// A formatted phrase as is supported by <see cref="string.Format(string,object[])" /> explaining why the assertion
/// is needed. If the phrase does not start with the word <i>because</i>, it is prepended automatically.
/// </param>
/// <param name="becauseArgs">
/// Zero or more objects to format using the placeholders in <see cref="because" />.
/// </param>
/// <remarks>Use this method to match the current <see cref="JToken"/> against an arbitrary subtree,
/// permitting it to contain any additional properties or elements. This way we can test multiple properties on a <see cref="JObject"/> at once,
/// or test if a <see cref="JArray"/> contains any items that match a set of properties, assert that a JSON document has a given shape, etc. </remarks>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also add the <example> snippets found on the other overloads here.

/// <code>
/// var json = JToken.Parse("{ success: true, data: { id: 123, type: 'my-type', name: 'Noone' } }");
/// json.Should().ContainSubtree(JToken.Parse("{ success: true, data: { type: 'my-type', name: 'Noone' } }"));
/// var json = JToken.Parse("{ success: true, data: { id: 123, type: 'my-type', name: 'None' } }");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: None is not more correct than Noone. (no one/no-one).
If tools think that Noone is a a typo , then let's just change it to a real name like John or Joe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ContainSubtree does not have an overload taking an options builder

4 participants