Visitar URL original
Fix for #521: Re-wrote `GivenSelector<T>.FailWith()` to match `AssertionChain.FailWith()` by JakenVeina · Pull Request #522 · AwesomeAssertions/AwesomeAssertions · GitHub
Skip to content

Fix for #521: Re-wrote GivenSelector<T>.FailWith() to match AssertionChain.FailWith() - #522

Merged
cbersch merged 1 commit into
AwesomeAssertions:mainfrom
JakenVeina:main
Jul 17, 2026
Merged

Fix for #521: Re-wrote GivenSelector<T>.FailWith() to match AssertionChain.FailWith()#522
cbersch merged 1 commit into
AwesomeAssertions:mainfrom
JakenVeina:main

Conversation

@JakenVeina

@JakenVeina JakenVeina commented Jun 28, 2026 •
edited by cbersch
Loading

Copy link
Copy Markdown
Contributor

Re-wrote the overload of GivenSelector<T>.FailWith() that accepts Func<> args to match the behavior of the equivalent overload of AssertionChain.FailWith().

The previous implementation seems to have been based on a faulty assumption of how AssertionChain.PreviousAssertionSucceeded works. Checking it does not ensure that logic doesn't run in the event of a failure of the current assertion that the .FailWith() call applies to, but rather the previous assertion in the chain (if any).

This implementation matches the logic of all the other .FailWith() implementations on GivenSelector<T>, as well as most of them on AssertionChain: it just wraps up the message-rendering logic and passes off the real work for dealing with fail/pass conditions to existing logic within AssertionChain.

Also added tests to cover this functionality, and confirm similar functionality in several overloads of .FailWith() for both AssertionChain and GivenSelector<T>.

Fix #521

IMPORTANT

  • If the PR touches the public API, the changes have been approved in a separate issue with the "api-approved" label.
  • The code complies with the Coding Guidelines for C#.
  • The changes are covered by unit tests which follow the Arrange-Act-Assert syntax and the naming conventions such as is used in these tests.
  • If the PR adds a feature or fixes a bug, please update the release notes with a functional description that explains what the change means to consumers of this library, which are published on the website.
  • If the PR changes the public API the changes needs to be included by running AcceptApiChanges.ps1 or AcceptApiChanges.sh.
  • If the PR affects the documentation, please include your changes in this pull request so the documentation will appear on the website.
    • Please also run ./build.sh --target spellcheck or .\build.ps1 --target spellcheck before pushing and check the good outcome

Legal checklist

  • This work is entirely original, it is not derived from any existing code incompatible with the Apache 2.0 License, like FluentAssertions.

@coveralls

coveralls commented Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown

Coverage Report for CI Build 28570000332

Coverage decreased (-0.02%) to 96.688%

Details

  • Coverage decreased (-0.02%) from the base build.
  • Patch coverage: 3 of 3 lines across 1 file are fully covered (100%).
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

Coverage Stats

Coverage Status
Relevant Lines: 12907
Covered Lines: 12752
Line Coverage: 98.8%
Relevant Branches: 4030
Covered Branches: 3624
Branch Coverage: 89.93%
Branches in Coverage %: Yes
Coverage Strength: 39090.4 hits per line
💛 - Coveralls

lg2de
lg2de reviewed Jun 29, 2026
View reviewed changes
Comment thread Tests/AwesomeAssertions.Specs/Execution/AssertionChainSpecs.Chaining.cs Resolved
@lg2de

lg2de commented Jun 30, 2026

Copy link
Copy Markdown
Member

I know that we have flaky code coverage some time. But there is at least one path now uncovered which could be related to the current change: https://coveralls.io/builds/80318278/source?filename=Src%2FAwesomeAssertions%2FExecution%2FContinuationOfGiven.cs
Please verify whether the coverage is really changed through your changes.

@IT-VBFK IT-VBFK linked an issue Jun 30, 2026 that may be closed by this pull request
Func<> args passed to GivenSelector<T>.FailWith() are invoked, even when not generating a message #521
Closed
@JakenVeina

JakenVeina commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

I know that we have flaky code coverage some time. But there is at least one path now uncovered which could be related to the current change: https://coveralls.io/builds/80318278/source?filename=Src%2FAwesomeAssertions%2FExecution%2FContinuationOfGiven.cs Please verify whether the coverage is really changed through your changes.

Far as I can tell, that property has precisely 0 references anywhere in the AwesomeAssertions codebase, on origin/main right now. Looking at the coveralls report of the prior build confirms, that line was not covered previously.

There does seem to be a problem with GivenSelector.cs, though, on the method that changed. If I'm reading the report right, coveralls is saying that the anonymous closure on line 74 is not actually invoked? I've added a few more tests to ensure coverage, assuming that's the case.

The report also says there's issues with CollectionAssertions.cs but that file refuses to load for me to look at them. I think it's just so big that coveralls is choking on it. Is that somehow just me? Maybe we'll get lucky and these are a byproduct of the missed delegate in GivenSelector.cs

@lg2de

lg2de commented Jul 1, 2026

Copy link
Copy Markdown
Member

I could not yet analyze myself whether the property is in use or not. Maybe we should remove it (some time).
Thanks for new tests, but some of them seems to fail.

Coverage changes on CollectionAssertions you can ignore here. This is the area where I've seen the flaky coverage in the past most time.

@JakenVeina
Fixed AwesomeAssertions#521: Re-wrote the overload of `GivenSelector<…
1de5413 
…T>.FailWith()` that accepts `Func<>` args to match the behavior of the equivalent overload of `AssertionChain.FailWith()`.

The previous implementation seems to have been based on a faulty assumption of how `AssertionChain.PreviousAssertionSucceeded` works. Checking it does not ensure that logic doesn't run in the event of a failure of the current assertion that the `.FailWith()` call applies to, but rather the previous assertion in the chain (if any).

This implementation matches the logic of all the other `.FailWith()` implementations on `GivenSelector<T>`, as well as most of them on `AssertionChain`: it just wraps up the message-rendering logic and passes off the real work for dealing with fail/pass conditions to existing logic within `AssertionChain`.
@JakenVeina

JakenVeina commented Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I promise I don't make a habit of submitting PRs without running tests. I must've gotten distracted by something while working on this, and overlooked it.

Updated and should be fixed now. Sorry for the wasted time.

@sonarqubecloud

sonarqubecloud Bot commented Jul 2, 2026

Copy link
Copy Markdown

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@lg2de

lg2de commented Jul 2, 2026

Copy link
Copy Markdown
Member

Once again we have the decreased coverage. I had a closer look on it today. I found that coveralls reports incomplete coverage in GivenSelector.cs:74. This is the first line of your new code.
It is not a total miss, but a branch miss. Unfortunately I do not have any idea why the two branches (which I can recognize) should not be covered.
I'm going to accept that as it is. What think @AwesomeAssertions/collaborators ?

@JakenVeina

JakenVeina commented Jul 4, 2026

Copy link
Copy Markdown
Contributor Author

I've confirmed with a debugger that one of my added tests (When_an_assertion_fails_it_should_invoke_argument_providers) hits all 3 branches within that method: the main branch, and both closures. So yeah, I dunno what coveralls is on about.

@IT-VBFK

IT-VBFK commented Jul 5, 2026

Copy link
Copy Markdown
Member

What think @AwesomeAssertions/collaborators ?

I think CC is just an informational tool. Sure, don't want to loose CC, but if it is e.g. massive or unrelated I'm fine by ignoring such warnings

cbersch
cbersch approved these changes Jul 17, 2026
View reviewed changes

@cbersch cbersch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for you contribution

@cbersch
cbersch merged commit 6896b32 into AwesomeAssertions:main Jul 17, 2026
7 of 8 checks passed
@cbersch cbersch added the enhancement Enhancement of existing feature label Jul 17, 2026
@cbersch cbersch mentioned this pull request Jul 19, 2026
Merged
8 tasks
This was referenced Jul 19, 2026
Merged
Merged
Merged
Merged
Closed
Merged
Merged
Merged
Merged
This was referenced Oct 26, 2026
Closed
Closed
Closed
Closed
Open
Open
Open
This was referenced Nov 2, 2026
Closed
Open
Merged
Merged
Closed
Open
Open
Open
This was referenced Nov 10, 2026
Open
Open
Closed
Closed
Closed
Closed
Closed
Closed
Open
Closed
Closed
Open
Merged
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cbersch cbersch cbersch approved these changes
@lg2de lg2de lg2de approved these changes
@IT-VBFK IT-VBFK IT-VBFK approved these changes

Assignees

No one assigned

Labels

enhancement Enhancement of existing feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Func<> args passed to GivenSelector<T>.FailWith() are invoked, even when not generating a message

5 participants

@JakenVeina @coveralls @lg2de @IT-VBFK @cbersch