-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Conversation
Tagging subscribers to this area: @tommcdon |
There was a problem hiding this 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.
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 Line 202 in b33f755
|
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. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
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! |
Is there a way to kill that |
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. |
/azp run runtime |
Azure Pipelines successfully started running 1 pipeline(s). |
/ba-g failure is dotnet/dnceng#3879 |
CodeVersionsTest
used manually implemented "mocks" before we integrationMoq
. Changes all tests to use theMoq
framework in accordance with newer tests.Previous comments on draft pr: max-charlamb#2