Skip to content
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

[cDAC] Use Moq in CodeVersionsTest #110750

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

max-charlamb
Copy link
Contributor

CodeVersionsTest used manually implemented "mocks" before we integration Moq. Changes all tests to use the Moq framework in accordance with newer tests.

Previous comments on draft pr: max-charlamb#2

Copy link
Contributor

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Copy link
Member

@ivdiazsa ivdiazsa left a comment

Choose a reason for hiding this comment

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

Are we sure we want to use Moq instead of NSubstitute? The former has had a history of poor security and potential spyware in more recent versions, while the latter is arguably easier to use, and is considered safe.

I would suggest really reconsidering the use of Moq in our codebase. If we really have to, then let's make sure we don't use a version newer than 4.18.4, which is the last one before these problems were encountered.

@max-charlamb
Copy link
Contributor Author

max-charlamb commented Dec 16, 2024

Moq is already used in several places in the repo and this PR brings some tests in line with existing usages.

If we want to have a broader discussion on switching to a different mocking framework, I'm open.

edit: It looks like we are pinned on version 4.18.4

<MoqVersion>4.18.4</MoqVersion>

@ivdiazsa
Copy link
Member

Moq is already used in several places in the repo and this PR brings some tests in line with existing usages.

If we want to have a broader discussion on switching to a different mocking framework, I'm open.

edit: It looks like we are pinned on version 4.18.4

<MoqVersion>4.18.4</MoqVersion>

That's good at least. We should be fine since we're sticking to the last not unsafe version, but I think moving away from it would be a good idea to bring up to discussion with others, as we can't update it anymore without putting our testing codebase at risk.

@elinor-fung
Copy link
Member

There was a fair amount of discussion around this last year. We started with staying on an older version: #90222 (comment)

I don't know where exactly we ended up more broadly, but I'd reach out to Rich if you want to find out or re-open the discussion.

Copy link
Member

@elinor-fung elinor-fung left a comment

Choose a reason for hiding this comment

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

Looks good!

@ivdiazsa
Copy link
Member

There was a fair amount of discussion around this last year. We started with staying on an older version: #90222 (comment)

I don't know where exactly we ended up more broadly, but I'd reach out to Rich if you want to find out or re-open the discussion.

I think it's worth discussing it internally again at some point after the holidays because after reading that thread, I have to say I'm left with a bad taste in my mouth and even bigger concerns about using that library. Thanks for pointing the context out @elinor-fung!

@ivdiazsa
Copy link
Member

Is there a way to kill that browser-wasm pipeline? It's frozen here but in AzDO it already shows an error that it was canceled by the remote provider. So we can run it again to get Build Analysis green and be able to merge.

@jkoritzinsky
Copy link
Member

Re Moq: I recommended Moq to Elinor because it's already referenced in the repo. Personally, I like FakeItEasy (which we also have already in dotnet-public and other repos use), but I didn't feel strongly enough about it to add the dependency. We should consider it as another option if we're reconsidering Moq usage.

@ivdiazsa
Copy link
Member

Re Moq: I recommended Moq to Elinor because it's already referenced in the repo. Personally, I like FakeItEasy (which we also have already in dotnet-public and other repos use), but I didn't feel strongly enough about it to add the dependency. We should consider it as another option if we're reconsidering Moq usage.

I just took a quick look at FakeItEasy and it seems like a great alternative option to consider. I agree, we should definitely suggest it next year when the Moq discussions are retaken.

@max-charlamb
Copy link
Contributor Author

/azp run runtime

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@max-charlamb
Copy link
Contributor Author

/ba-g failure is dotnet/dnceng#3879

@max-charlamb max-charlamb merged commit 55763b8 into dotnet:main Jan 8, 2025
146 of 148 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Diagnostics-coreclr test-enhancement Improvements of test source code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants