Introduce new assertions on ParameterInfo #2385

0xced · 2023-10-14T21:31:28Z

All the (Not)BeDecoratedWith methods were implemented for MemberInfo but implementing them for ICustomAttributeProvider instead allows for a free ParameterInfoAssertions implementation.

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

github-actions · 2023-10-14T21:36:43Z

Qodana for .NET

9 new problems were found

Inspection name	Severity	Problems
`Possible multiple enumeration`	🔶 Warning	8
`Redundant using directive`	🔶 Warning	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

coveralls · 2023-10-14T21:53:35Z

Pull Request Test Coverage Report for Build 6676121464

84 of 159 (52.83%) changed or added relevant lines in 10 files are covered.
4 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.6%) to 96.8%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
Src/FluentAssertions/Specialized/AssemblyAssertions.cs	0	1	0.0%
Src/FluentAssertions/Types/MemberInfoAssertions.cs	0	1	0.0%
Src/FluentAssertions/Types/ParameterInfoAssertions.cs	4	5	80.0%
Src/FluentAssertions/AssertionExtensions.cs	1	4	25.0%
Src/FluentAssertions/TypeExtensions.cs	0	4	0.0%
Src/FluentAssertions/Types/ParameterInfoSelectorAssertions.cs	0	29	0.0%
Src/FluentAssertions/Types/ParameterInfoSelector.cs	0	36	0.0%

Files with Coverage Reduction	New Missed Lines	%
Src/FluentAssertions/Types/MemberInfoAssertions.cs	1	50.0%
Src/FluentAssertions/Common/TypeExtensions.cs	3	96.86%

Totals
Change from base Build 6663997299:	-0.6%
Covered Lines:	11698
Relevant Lines:	11965

💛 - Coveralls

dennisdoomen

🤔 We now miss a Parameters() extension method like we have in our Reflection API

dennisdoomen · 2023-10-15T07:45:09Z

Src/FluentAssertions/Types/CustomAttributeProviderAssertions.cs

+/// Contains a number of methods to assert that a <see cref="ICustomAttributeProvider"/> is in the expected state.
+/// </summary>
+[DebuggerNonUserCode]
+public abstract class CustomAttributeProviderAssertions<TSubject, TAssertions> : ReferenceTypeAssertions<TSubject, TAssertions>


🤔 Given that this type may contain other assertions that are reusable across the reflection assertions, I think we should name this something like ReflectionAssertions
🌱 I hope using inheritance isn't going to cause issues later on.

Currently our inheritance hierarchy of type-related assertions follows the inheritance hierarchy of the corresponding BCL types.
I notice that e.g. Assembly also implements ICustomAttributeProvider.

How difficult would it be to implement this feature without changing our inheritance hierarchy?

It's not show stopper for me, it just feels a little odd to me to mix these concepts.

dennisdoomen · 2023-10-15T07:46:14Z

Tests/FluentAssertions.Specs/Types/ParameterInfoAssertionSpecs.cs

+
+public class ParameterInfoAssertionSpecs
+{
+    public class BeDecortatedWithOfT


⛏️ Typo

dennisdoomen · 2023-10-15T07:53:09Z

Tests/FluentAssertions.Specs/Types/ParameterInfoAssertionSpecs.cs

+    public class BeDecortatedWithOfT
+    {
+        [Fact]
+        public void When_asserting_a_parameter_is_decorated_with_attribute_and_it_is_it_succeeds()


🔧 We have been adopting a more fact-based naming convention for new tests, e.g. A_parameter_that_is_decorated_with_the_expected_attribute_is_valid or Succeeds_for_a_parameter_with_the_expected_attribute. For example, It's completely clear from the parent class that every test is about checking whether a parameter is decorated with an attribute, so you can leave that out. Same for the other tests.

dennisdoomen · 2023-10-15T07:56:17Z

Tests/FluentAssertions.Specs/Types/ParameterInfoAssertionSpecs.cs

+        }
+
+        [Fact]
+        public void When_subject_is_null_be_decorated_withOfT_should_fail()


🔧 Stay away from technical terms like OfT

dennisdoomen · 2023-10-15T07:56:34Z

Tests/FluentAssertions.Specs/Types/ParameterInfoAssertionSpecs.cs

+        }
+    }
+
+    #region Internal classes used in unit tests


🔧 We prefer not to use regions anymore.

jnyrup · 2023-10-15T09:55:56Z

Src/FluentAssertions/Types/CustomAttributeProviderAssertions.cs

+    /// <param name="becauseArgs">
+    /// Zero or more objects to format using the placeholders in <paramref name="because" />.
+    /// </param>
+    public AndWhichConstraint<CustomAttributeProviderAssertions<TSubject, TAssertions>, TAttribute> BeDecoratedWith<TAttribute>(


This should return AndWhichConstraint<TAssertions, TAttribute> such that one continue chaining with methods defined on a derived type.

jnyrup · 2023-10-15T10:18:16Z

Src/FluentAssertions/Types/CustomAttributeProviderAssertions.cs

+/// Contains a number of methods to assert that a <see cref="ICustomAttributeProvider"/> is in the expected state.
+/// </summary>
+[DebuggerNonUserCode]
+public abstract class CustomAttributeProviderAssertions<TSubject, TAssertions> : ReferenceTypeAssertions<TSubject, TAssertions>


Currently our inheritance hierarchy of type-related assertions follows the inheritance hierarchy of the corresponding BCL types.
I notice that e.g. Assembly also implements ICustomAttributeProvider.

How difficult would it be to implement this feature without changing our inheritance hierarchy?

It's not show stopper for me, it just feels a little odd to me to mix these concepts.

jnyrup · 2023-10-15T10:23:41Z

Src/FluentAssertions/Common/TypeExtensions.cs

-        // Do not use MemberInfo.IsDefined
-        // There is an issue with PropertyInfo and EventInfo preventing the inherit option to work.
-        // https://github.com/dotnet/runtime/issues/30219
-        return Attribute.IsDefined(type, typeof(TAttribute), inherit: false);
+        return type.IsDefined(typeof(TAttribute), inherit: false);


It seems this will be calling the MemberInfo.IsDefined that the comment said not to use.
I dug into this and the related PRs are #748 and #1095.

I checked out develop at #748 and used the prohibited method.
I don't have net45 or netcoreapp1.2 installed, but for the remaining TFMs no tests failed.
Since we no longer support these TFMs directly or indirectly, I think it's safe to use the ICustomAttributeProvider.IsDefined.

0xced · 2023-10-15T18:22:53Z

I won't be able to address all the feedback in the coming weeks so please don't hesitate to adapt everything that needs to be adapted if have the time to.

Also, note that all the specs were mostly copy/pasted from PropertyInfoAssertionSpecs.

0xced · 2023-10-26T13:40:12Z

Wow, I did not realize that this pull request was opening Pandora's box! 😅

Indeed a new Parameters() extension method would be warranted. This also implies designing all the properties that would make sense on a ParameterInfoSelector (e.g. ThatAreInput, ThatHaveDefaultValue etc.) Would you prefer me to add it as part of this pull request or in a follow-up pull request?

And yes, Assembly also implements ICustomAttributeProvider, so does Type! 😁

I'm currently reading Multiple Inheritance in C# on Stack Overflow to see if I can come up with an elegant solution that does not involve changing the inheritance hierarchy.

In the end, that's quite some work ahead for something that was supposed to just be some support for #2380. 😂

dennisdoomen · 2023-10-26T19:27:33Z

Wow, I did not realize that this pull request was opening Pandora's box!

Sorry about that. This tends to happen with an open-source library that has close to 300 million downloads ;-).

In the end, that's quite some work ahead for something that was supposed to just be some support for #2380. 😂

You could still decouple #2380 from this one.

IT-VBFK · 2023-10-28T10:34:24Z

Think about coverage and qodana scan, if you can :)

dennisdoomen · 2024-01-06T09:49:55Z

@0xced did you give up on this?

0xced · 2024-01-13T23:51:30Z

I did not give up on this but since that was just a side effect of #2380 and not something I actually need I did not gave it a high priority.

github-actions · 2024-01-13T23:57:33Z

Qodana for .NET

10 new problems were found

Inspection name	Severity	Problems
`Possible multiple enumeration`	🔶 Warning	8
`Redundant using directive`	🔶 Warning	2

💡 Qodana analysis was run in the pull request mode: only the changed files were checked
☁️ View the detailed Qodana report

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

All the (Not)BeDecoratedWith(OrInherit) methods are now available on all types that support attributes, i.e. `Assembly`, `MemberInfo`, `ParameterInfo` and `Type`.

TODO: ThatAre... (IsLcid, IsOptional, etc.)

0xced mentioned this pull request Oct 14, 2023

Add [NotNull] attribute on the Should() method for object assertions #2380

Merged

0xced force-pushed the ParameterInfoAssertions branch from 1e86698 to c9f6d16 Compare October 14, 2023 21:44

dennisdoomen requested changes Oct 15, 2023

View reviewed changes

dennisdoomen added the enhancement label Oct 15, 2023

jnyrup reviewed Oct 15, 2023

View reviewed changes

0xced force-pushed the ParameterInfoAssertions branch 3 times, most recently from 3a11e43 to 17d9331 Compare October 26, 2023 17:20

0xced force-pushed the ParameterInfoAssertions branch 3 times, most recently from 4c33b31 to 632a02a Compare October 28, 2023 09:34

0xced force-pushed the ParameterInfoAssertions branch from 632a02a to 1dbfb54 Compare January 13, 2024 23:42

0xced added 2 commits January 14, 2024 10:41

Introduce new assertions on ParameterInfo

1554e72

All the (Not)BeDecoratedWith(OrInherit) methods are now available on all types that support attributes, i.e. `Assembly`, `MemberInfo`, `ParameterInfo` and `Type`.

[WIP] ParameterInfoSelector

2c0c67e

TODO: ThatAre... (IsLcid, IsOptional, etc.)

0xced force-pushed the ParameterInfoAssertions branch from 1dbfb54 to 2c0c67e Compare January 14, 2024 09:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce new assertions on ParameterInfo #2385

Introduce new assertions on ParameterInfo #2385

0xced commented Oct 14, 2023

github-actions bot commented Oct 14, 2023 •

edited

coveralls commented Oct 14, 2023 •

edited

dennisdoomen left a comment

dennisdoomen Oct 15, 2023

jnyrup Oct 15, 2023

dennisdoomen Oct 15, 2023

dennisdoomen Oct 15, 2023

dennisdoomen Oct 15, 2023

dennisdoomen Oct 15, 2023

jnyrup Oct 15, 2023

jnyrup Oct 15, 2023

jnyrup Oct 15, 2023

0xced commented Oct 15, 2023

0xced commented Oct 26, 2023

dennisdoomen commented Oct 26, 2023

IT-VBFK commented Oct 28, 2023

dennisdoomen commented Jan 6, 2024

0xced commented Jan 13, 2024

github-actions bot commented Jan 13, 2024 •

edited

Introduce new assertions on ParameterInfo #2385

Are you sure you want to change the base?

Introduce new assertions on ParameterInfo #2385

Conversation

0xced commented Oct 14, 2023

IMPORTANT

github-actions bot commented Oct 14, 2023 • edited

Qodana for .NET

coveralls commented Oct 14, 2023 • edited

Pull Request Test Coverage Report for Build 6676121464

💛 - Coveralls

dennisdoomen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xced commented Oct 15, 2023

0xced commented Oct 26, 2023

dennisdoomen commented Oct 26, 2023

IT-VBFK commented Oct 28, 2023

dennisdoomen commented Jan 6, 2024

0xced commented Jan 13, 2024

github-actions bot commented Jan 13, 2024 • edited

Qodana for .NET

github-actions bot commented Oct 14, 2023 •

edited

coveralls commented Oct 14, 2023 •

edited

github-actions bot commented Jan 13, 2024 •

edited