Skip to content

Fix attribute order dependency in ModelAttributesTest #62909

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 25, 2025

Conversation

medhatiwari
Copy link
Contributor

This PR fixes a test failure on Mono runtime by removing dependency on attribute ordering in ModelAttributesTest.GetAttributesForParameter_SomeAttributes

Description

The test was failing on Mono runtime because it assumed SerializableAttribute would be the first attribute returned by GetCustomAttributes()

Failed Microsoft.AspNetCore.Mvc.ModelBinding.ModelAttributesTest.GetAttributesForParameter_SomeAttributes
Error: Assert.IsType() Failure: Value is not the exact type
Expected: typeof(System.SerializableAttribute) 
Actual:   typeof(System.Runtime.CompilerServices.IsReadOnlyAttribute)

see detail here

As pointed out by @jkotas in dotnet/runtime#117864:

"The order of attributes is not significant. [...] If the test depends on attributes being returned in a specific order, it should be fixed to not do that."

The C# Specification confirms that attribute order is not guaranteed

This change ensures the test verifies the presence of the expected attribute without depending on its position, making it compliant with the C# specification and compatible across all .NET runtimes.

cc: @jkotas

@Copilot Copilot AI review requested due to automatic review settings July 24, 2025 18:50
@medhatiwari medhatiwari requested a review from a team as a code owner July 24, 2025 18:50
Copy link
Contributor

@Copilot 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 fixes a test failure on Mono runtime by removing the dependency on attribute ordering in ModelAttributesTest.GetAttributesForParameter_SomeAttributes. The test was failing because it assumed SerializableAttribute would always be the first attribute returned by GetCustomAttributes(), but attribute order is not guaranteed by the C# specification.

Key Changes

  • Replaced order-dependent Assert.Collection with order-agnostic Assert.Contains check
  • Updated test to verify presence of SerializableAttribute without assuming its position
  • Added explanatory comment about the order-agnostic nature of the check

@github-actions github-actions bot added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jul 24, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 24, 2025
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM!

@captainsafia captainsafia merged commit bcfa508 into dotnet:main Jul 25, 2025
31 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-rc1 milestone Jul 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants