Fix issue when an empty ArraySegment is a member of a class. by ShaharPrishMSFT · Pull Request #2445 · fluentassertions/fluentassertions

ShaharPrishMSFT · 2023-11-08T10:27:52Z

With latest, it crashes when trying to access the array members of an empty ArraySegment.

How do I change the release notes? Where do I add the bug fix?

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

With latest, it crashes when trying to access the array members.

github-actions · 2023-11-08T10:34:50Z

Qodana for .NET

2 new problems were found

Inspection name	Severity	Problems
`Redundant using directive`	🔶 Warning	1
`Remove redundant parentheses`	◽️ Notice	1

💡 Qodana analysis was run in the pull request mode: only the changed files were checked

View the detailed Qodana report

To be able to view the detailed Qodana report, you can either:

Register at Qodana Cloud and configure the action
Use GitHub Code Scanning with Qodana
Host Qodana report at GitHub Pages
Inspect and use qodana.sarif.json (see the Qodana SARIF format for details)

To get *.log files or any other Qodana artifacts, run the action with upload-result option set to true,
so that the action will upload the files as the job artifacts:

      - name: 'Qodana Scan'
        uses: JetBrains/qodana-action@v2023.2.8
        with:
          upload-result: true

Contact Qodana team

Contact us at qodana-support@jetbrains.com

Or via our issue tracker: https://jb.gg/qodana-issue
Or share your feedback: https://jb.gg/qodana-discussions

dennisdoomen

Please also check out the contribution guidelines on release notes.

Src/FluentAssertions/Equivalency/Steps/EnumerableEquivalencyStep.cs

Tests/FluentAssertions.Equivalency.Specs/TestTypes.cs

ITaluone · 2023-11-09T09:36:48Z

Typo in PR title ;) ArraySegmnet -> ArraySegment

Updated to include an ArraySegment fix. fluentassertions#2445

ShaharPrishMSFT · 2023-11-09T10:29:17Z

Please also check out the contribution guidelines on release notes.

Fixed: develop...ShaharPrishMSFT:fluentassertions:patch-1

ShaharPrishMSFT · 2023-11-09T10:29:48Z

Typo in PR title ;) ArraySegmnet -> ArraySegment

Fixed and addressed comments in the PR. Please take another look.

ShaharPrishMSFT · 2023-11-09T11:37:11Z

@dennisdoomen

Can I see what the issues found by Qodana is without registering to something?

It gives the errors, but not where they are.

ITaluone · 2023-11-09T11:44:50Z

Since the Qodana scan does not include the direct link to the report anymore.. it is a little pain in the ass ;)

click on "Details" besides the qodana run
un-fold the "Qodana Scan" in the logs
scoll to the bottom until you see something like this:
the qodana.cloud/XXXXXX is the link to go

https://qodana.cloud/projects/AgRgk/reports/M6a24

Src/FluentAssertions/Equivalency/Steps/EnumerableEquivalencyStep.cs

ITaluone · 2023-11-09T13:21:45Z

Also don't forget about a mention in the release notes :)

ShaharPrishMSFT · 2023-11-09T15:10:50Z

Also don't forget about a mention in the release notes :)

Did :)

ShaharPrishMSFT · 2023-11-09T15:11:37Z

@ITaluone / @dennisdoomen

I don't contribute often, so not sure if there's anything else I need to do here.

ITaluone · 2023-11-09T15:29:17Z

Did :)

No? I see only two files changed?

And btw.. new qodana issues ;)

dennisdoomen · 2023-11-09T15:44:11Z

don't contribute often, so not sure if there's anything else I need to do here.

Update the releases.md
Fix the Qodana issues

coveralls · 2023-11-09T15:52:51Z

Pull Request Test Coverage Report for Build 6811806552

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.03%) to 97.426%

Totals
Change from base Build 6773593369:	0.03%
Covered Lines:	11744
Relevant Lines:	11929

💛 - Coveralls

ShaharPrishMSFT · 2023-11-10T11:42:16Z

don't contribute often, so not sure if there's anything else I need to do here.

Update the releases.md

Fix the Qodana issues

I thought I did... I sent a link. What else am I missing here? develop...ShaharPrishMSFT:fluentassertions:patch-1
They all seem to be fixed, no?

dennisdoomen · 2023-11-10T12:37:40Z

I thought I did... I sent a link. What else am I missing here? develop...ShaharPrishMSFT:fluentassertions:patch-1

Ah, no, you're supposed to do that as part of this PR. Then, when we release the change, the docs will be in sync with the code.

dennisdoomen · 2023-11-10T12:38:44Z

2. They all seem to be fixed, no?

No, they are not (yet). Notice the little black box. It isn't a green checkbox.

jnyrup

I tried to search the BCL for other types having this behavior, but couldn't find others, so I'm fine with hack rather than finding a more general solution.

jnyrup · 2023-11-11T15:21:18Z

Src/FluentAssertions/Equivalency/Steps/EnumerableEquivalencyStep.cs

+        return value.GetType() is { } type &&
+                    (type.Name.Equals("ImmutableArray`1", StringComparison.Ordinal) ||
+                     (type.IsGenericType && type.GetGenericTypeDefinition() == typeof(ArraySegment<>)));


I would prefer either of these two

private static bool IsIgnorableArrayLikeType1(object value) { var type = value.GetType(); return type.Name.Equals("ImmutableArray`1", StringComparison.Ordinal) || (type.IsGenericType && type.GetGenericTypeDefinition() == typeof(ArraySegment<>)); } private static bool IsIgnorableArrayLikeType2(object value) { return value.GetType().Name.Equals("ImmutableArray`1", StringComparison.Ordinal) || (value.GetType().IsGenericType && value.GetType().GetGenericTypeDefinition() == typeof(ArraySegment<>)); }

All three variants JITs to the same assembly code

SharpLab

Addressed in #2511

ITaluone · 2023-11-14T11:58:37Z

Tests/FluentAssertions.Equivalency.Specs/BasicSpecs.cs

+        // Act
+        Action act = () => actual.Should().BeEquivalentTo(expected);
+
+        // Assert
+        act.Should().NotThrow();


Suggested change

// Act

Action act = () => actual.Should().BeEquivalentTo(expected);

// Assert

act.Should().NotThrow();

// Act / Assert

actual.Should().BeEquivalentTo(expected);

Just call the assertion. If it fails, it fails anyway.

Addressed in #2511

dennisdoomen · 2023-11-23T06:59:50Z

@ShaharPrishMSFT did you give up on us?

ITaluone · 2023-11-30T10:12:39Z

I am willing to finish this, if @ShaharPrishMSFT does not want to. :)

Edit: I will wait until 8th of december :)

Updated to include an ArraySegment fix. fluentassertions#2445

Fix issue when an empty ArraySegmnet is a member of a class.

f720aa7

With latest, it crashes when trying to access the array members.

ShaharPrishMSFT mentioned this pull request Nov 8, 2023

Types that contain an empty ArraySegment<>s cannot be checked with IsEquivalentTo. #2446

Closed

dennisdoomen requested changes Nov 8, 2023

View reviewed changes

Src/FluentAssertions/Equivalency/Steps/EnumerableEquivalencyStep.cs Outdated Show resolved Hide resolved

Tests/FluentAssertions.Equivalency.Specs/TestTypes.cs Outdated Show resolved Hide resolved

Fix PR comments.

c329b1f

ShaharPrishMSFT changed the title ~~Fix issue when an empty ArraySegmnet is a member of a class.~~ Fix issue when an empty ArraySegment is a member of a class. Nov 9, 2023

ShaharPrishMSFT added a commit to ShaharPrishMSFT/fluentassertions that referenced this pull request Nov 9, 2023

Update releases.md

1c64370

Updated to include an ArraySegment fix. fluentassertions#2445

Fi Qordana report.

a99f89f

ITaluone reviewed Nov 9, 2023

View reviewed changes

Src/FluentAssertions/Equivalency/Steps/EnumerableEquivalencyStep.cs Outdated Show resolved Hide resolved

Fix again.

5ff6f2b

jnyrup requested changes Nov 11, 2023

View reviewed changes

jnyrup added the bug label Nov 11, 2023

ITaluone reviewed Nov 14, 2023

View reviewed changes

ITaluone pushed a commit to ITaluone/fluentassertions that referenced this pull request Dec 12, 2023

Update releases.md

ec59d77

Updated to include an ArraySegment fix. fluentassertions#2445

Conversation

ShaharPrishMSFT commented Nov 8, 2023

IMPORTANT

Uh oh!

github-actions bot commented Nov 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Qodana for .NET

Uh oh!

dennisdoomen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ITaluone commented Nov 9, 2023

Uh oh!

ShaharPrishMSFT commented Nov 9, 2023

Uh oh!

ShaharPrishMSFT commented Nov 9, 2023

Uh oh!

ShaharPrishMSFT commented Nov 9, 2023

Uh oh!

ITaluone commented Nov 9, 2023

Uh oh!

Uh oh!

ITaluone commented Nov 9, 2023

Uh oh!

ShaharPrishMSFT commented Nov 9, 2023

Uh oh!

ShaharPrishMSFT commented Nov 9, 2023

Uh oh!

ITaluone commented Nov 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dennisdoomen commented Nov 9, 2023

Uh oh!

coveralls commented Nov 9, 2023

Pull Request Test Coverage Report for Build 6811806552

Warning: This coverage report may be inaccurate.

💛 - Coveralls

Uh oh!

ShaharPrishMSFT commented Nov 10, 2023

Uh oh!

dennisdoomen commented Nov 10, 2023

Uh oh!

dennisdoomen commented Nov 10, 2023

Uh oh!

jnyrup left a comment

Choose a reason for hiding this comment

Uh oh!

jnyrup Nov 11, 2023

Choose a reason for hiding this comment

Uh oh!

ITaluone Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

ITaluone Nov 14, 2023

Choose a reason for hiding this comment

Uh oh!

ITaluone Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

dennisdoomen commented Nov 23, 2023

Uh oh!

ITaluone commented Nov 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

github-actions bot commented Nov 8, 2023 •

edited

Loading

ITaluone commented Nov 9, 2023 •

edited

Loading

ITaluone commented Nov 30, 2023 •

edited

Loading