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

Add "set" methods to ProtectedAsMock #1165

Merged
merged 7 commits into from Jul 22, 2021

Conversation

tonyhallett
Copy link
Contributor

Fix #1164

Copy link
Contributor

@stakx stakx left a comment

Choose a reason for hiding this comment

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

Apologies for keeping you waiting so long. I've been rather busy at work.

Good work! 👍 I'd be happy if you could make a few small changes (as per the review comments). Also, could you please add an entry to CHANGELOG.md in the Unreleased section, for instance:

 ## Unreleased

+#### Added
+
+* `SetupSet`, `VerifySet` methods for `mock.Protected().As<>()` (@tonyhallett, #1165)

 #### Fixed

Thank you!

src/Moq/Protected/ProtectedAsMock.cs Outdated Show resolved Hide resolved
src/Moq/Protected/IProtectedAsMock.cs Outdated Show resolved Hide resolved
src/Moq/Protected/ProtectedAsMock.cs Outdated Show resolved Hide resolved
src/Moq/Protected/DuckSetterReplacer.cs Outdated Show resolved Hide resolved
src/Moq/Protected/DuckSetterReplacer.cs Outdated Show resolved Hide resolved
src/Moq/Protected/IProtectedAsMock.cs Outdated Show resolved Hide resolved
src/Moq/Protected/IProtectedAsMock.cs Outdated Show resolved Hide resolved
tests/Moq.Tests/ProtectedAsMockFixture.cs Show resolved Hide resolved
tests/Moq.Tests/ProtectedAsMockFixture.cs Outdated Show resolved Hide resolved
tests/Moq.Tests/ProtectedAsMockFixture.cs Outdated Show resolved Hide resolved
@tonyhallett
Copy link
Contributor Author

tonyhallett commented Jun 23, 2021

@stakx
Changes made. There is now one duck replacer class that now correctly accounts for virtual protected accessors and automocking #1162

Once this has been approved I will update the draft InSequence for protected mocks pull request #1174

Copy link
Contributor

@stakx stakx left a comment

Choose a reason for hiding this comment

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

Hi @tonyhallett, thanks for making the requested changes!

I'm a little confused by the additional changes you've made. Can we please do the bugfixing of auto-mocking and virtual properties in one or more separate focused PRs... or is this somehow a necessary consequence of mixing your replacer class into the existing DuckReplacer?

CHANGELOG.md Outdated Show resolved Hide resolved
tests/Moq.Tests/ProtectedAsMockFixture.cs Outdated Show resolved Hide resolved
src/Moq/Protected/ProtectedAsMock.cs Outdated Show resolved Hide resolved
src/Moq/Protected/ProtectedAsMock.cs Outdated Show resolved Hide resolved
tests/Moq.Tests/ProtectedAsMockFixture.cs Outdated Show resolved Hide resolved
src/Moq/Protected/ProtectedAsMock.cs Outdated Show resolved Hide resolved
tests/Moq.Tests/ProtectedAsMockFixture.cs Outdated Show resolved Hide resolved
tests/Moq.Tests/ProtectedAsMockFixture.cs Outdated Show resolved Hide resolved
tests/Moq.Tests/ProtectedAsMockFixture.cs Outdated Show resolved Hide resolved
src/Moq/Protected/ProtectedAsMock.cs Outdated Show resolved Hide resolved
@stakx
Copy link
Contributor

stakx commented Jul 19, 2021

@tonyhallett, thanks for extracting the bug fix into a separate PR #1185. (Good work!) I've marked those review comments that were related to it as resolved, so you can now proceed with rebasing & cleaning up this PR; there are now only a few details remaining. I'll try my best to respond more quickly this time!

@stakx stakx added this to the 4.16.2 milestone Jul 19, 2021
@tonyhallett
Copy link
Contributor Author

@stakx You ok if I squash these commits into one ?

@stakx
Copy link
Contributor

stakx commented Jul 21, 2021

@tonyhallett, sure, go ahead. Let me know once it's ready to be merged.

@tonyhallett
Copy link
Contributor Author

@stakx

You ok if I squash these commits into one ?

Unfortunately this has highlighted my poor knowledge of git. I assume I need --rebase-merges....
Please can you assist ?

@stakx
Copy link
Contributor

stakx commented Jul 22, 2021

@tonyhallett, if you want, I can do the squashing when I merge (GitHub's web UI has an option to squash-merge a PR). That would probably be easiest and quickest.

If you want to do it yourself, for learning purposes, you can do so using an interactive rebase (git rebase -i):

git checkout protectedasmock-set-methods
git rebase -i main  # assuming that you forked `protectedasmock-set-methods` off the `main` branch
# in the editor that opens, change all except the first `pick` command to `squash`
# then save and exit the editor

If you want to ensure that your branch is current with respect to this repository (moq/moq4)'s main branch, you could additionally do the following first:

git remote add upstream https://github.com/moq/moq4.git
git checkout main
git pull upstream/main

This should fast-forward your main branch so it's in sync with moq/moq4's main branch. Then follow it up with the interactive rebase described above.

@tonyhallett
Copy link
Contributor Author

if you want, I can do the squashing when I merge (GitHub's web UI has an option to squash-merge a PR). That would probably be easiest and quickest.

Agreed. Please do

@stakx stakx merged commit 9e148f6 into devlooped:main Jul 22, 2021
@stakx
Copy link
Contributor

stakx commented Jul 22, 2021

Done. 🚀

Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing setter functionality from ProtectedAsMock
2 participants