Skip to content

Update MoqVersion to latest#66540

Merged
Youssef1313 merged 4 commits into
mainfrom
dev/ygerges/update-moq
May 4, 2026
Merged

Update MoqVersion to latest#66540
Youssef1313 merged 4 commits into
mainfrom
dev/ygerges/update-moq

Conversation

@Youssef1313
Copy link
Copy Markdown
Member

@Youssef1313 Youssef1313 commented Apr 30, 2026

This version of Moq is extremely old.

During the test run of Microsoft.AspNetCore.Mvc.TagHelpers.Test, I see lots of threadpool threads getting blocked:

image image

This doesn't sound good, and might be cause of some of the flakiness we get.

Let's hope that the newer versions of Moq/Castle.Core are doing better job at not blocking threads.

Copilot AI review requested due to automatic review settings April 30, 2026 12:24
@Youssef1313 Youssef1313 requested review from a team and wtgodbe as code owners April 30, 2026 12:24
@github-actions github-actions Bot added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Apr 30, 2026
Copy link
Copy Markdown
Contributor

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

Updates the centrally-managed Moq dependency version used across the repo’s test builds (via custom <Reference><PackageReference> resolution), aiming to pick up fixes/improvements from a much newer Moq release.

Changes:

  • Bump MoqVersion from 4.10.0 to 4.20.72.

@wtgodbe
Copy link
Copy Markdown
Member

wtgodbe commented Apr 30, 2026

Looks like there might be some reaction needed in some of the MVC tests (Microsoft.AspNetCore.Mvc.ModelBinding.Validation.DefaultObjectValidatorTests.Validate_ComplexType_IValidatableObject_CanUseRequestServices, for example):

Moq.MockException : Mock<DefaultObjectValidatorTests.IExampleService:1>:
This mock failed verification due to the following:

DefaultObjectValidatorTests.IExampleService x => x.DoSomething():
This setup was not matched.

@Youssef1313
Copy link
Copy Markdown
Member Author

This test passes a mock for validation, and it's now failing the validation. I think the difference might be around this:

image

I will need to dig further next Monday. But my feeling is that the test should simply avoid passing Mock<T>.Object if the validation logic goes to deep into that object and is surfacing implementation details of Moq (I assume the validation logic tries to do deep validation with reflection). Instead of creating a Mock<IValidatableObject> with Moq, the test could simply just create class MockedValidatableObject : IValidatableObject.

@Youssef1313
Copy link
Copy Markdown
Member Author

Youssef1313 commented May 1, 2026

The path how we get there:

  • Validation of Mock<IValidatableObject>.Object visits the object. It's seen as complex type, so we visit complex type with DefaultComplexObjectValidationStrategy.Instance.
  • Note that the Mock<T>.Object is IMocked<T> and has a Mock property that seems to refer back to the original Mock<IValidatableObject>.
  • We visit the children, and specifically the Mock<T> (coming through the Mock property).
  • We visit the children of that, and Mock.Setups (SetupCollection) is one of those.
  • The setup collection passes the IsEnumerableType check so we call into VisitComplexType(DefaultCollectionValidationStrategy.Instance).
  • We then visit Setups[0] which is Moq.MethodCall representing the single setup that the test adds (x => x.Validate(It.IsAny<ValidationContext>())).
  • The MethodCall is complex type that we visit and then validate its children.
  • InnerMock is one of those children that we visit.
  • The InnerMock appears to be null - and fails RequiredAttribute validation.

The reason we validate against ReuiqredAttribute in this case is that it's inferred via nullability annotations. So, IsRequired implementation returns true, then addInferredRequiredAttribute is true and then we fakely add RequiredAttribute instance.

image

As you see, the validation result can be highly impacted by the implementation details of Moq and we should avoid that in the test altogether, IMO. Meanwhile, devlooped/moq#1672

@Youssef1313 Youssef1313 requested a review from a team as a code owner May 1, 2026 17:12
@Youssef1313 Youssef1313 force-pushed the dev/ygerges/update-moq branch from 79081e6 to b178af7 Compare May 1, 2026 17:12
@Youssef1313
Copy link
Copy Markdown
Member Author

Youssef1313 commented May 1, 2026

We're still suffering from threadpool starvation, I think, but the update still feels like a good step forward.

image

We should still, IMO, find ways to improve this. That might require changes on Moq/Castle sides (castleproject/Core#743). Or alternatively, we try to simply reduce our usage of Moq whenever possible.

@Youssef1313 Youssef1313 enabled auto-merge (squash) May 4, 2026 09:22
@Youssef1313 Youssef1313 merged commit 80a94e2 into main May 4, 2026
25 checks passed
@Youssef1313 Youssef1313 deleted the dev/ygerges/update-moq branch May 4, 2026 13:23
@dotnet-policy-service dotnet-policy-service Bot added this to the 11.0-preview5 milestone May 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants