[Naming] System under Test should not be called "mock"

[fleupold]

When a solidity library is unit tested, it needs to be wrapped in a smart contract in order to be exposed for the js unit testing framework. The OpenZeppelin framework chose to call these wrapping contracts Mocks, e.g.

https://github.com/OpenZeppelin/openzeppelin-solidity/blob/b644c72eb083d7a84a5c9924ec8012a7c29290fd/contracts/mocks/MathMock.sol#L7-L19

They are used in unit tests like this:

https://github.com/OpenZeppelin/openzeppelin-solidity/blob/20cf88543024fb95d79db16a2d7a3d1c32090e2a/test/library/Math.test.js#L13-L15

I think the name mock is quite confusing in this context. In traditional unit testing mocks are used to simulate behaviour of complex dependencies that the system under test (SUT) depends on.
Here we are calling the SUT itself a mock, which seems like an anti-pattern. The SUT should never be mocked, as it's the thing we are testing.

To avoid confusion, would it be acceptable to rename the contracts that are wrapping the libraries, e.g. instead of MathMock something like MathProxy, MathFacade or MathWrapper?

[nventuro]

Yes, I think we all agree on this - mock is a very poor name. Keep in mind this is not the only scenario in which we use them: we also have mocks for abstract contracts, which complete the base implementation (e.g. ConditionalEscrow), and mocks for contracts that provide modifiers, so that they can be tested (e.g. Ownable). Would you call all of these by the same name?

[fleupold]

I wasn't able to find the OwnableMock.

The ConditionalEscrowMock is I believe more what people would call a spy (at least in Mockito terminology), since it stubs some behaviour but leaves the rest of the object intact.

Interestingly, there is also already a MerkleProofWrapper.

Another naming suggestion we came up while thinking about this more was to use Exposed. We could add the type of functionality we are exposing as well (e.g. MathLibraryExposed, OwnableModifierExposed).

[nventuro]

You're right regarding OwnableMock - I've just checked and we have no tests for that modifier! Yikes. You can take a look at PausableMock though to see what I meant.

I think Exposed makes a lot of sense for libraries and e.g. MinterRole, where we need to expose internal methods. I worry a bit that having multiple names for this kind of thing (e.g. Spy, Exposed, something else for the ones with modifiers, etc.) may cause confusion though.

[shrugs]

I agree that mocks are not the right term for creating SUTs; we use the Impl for some cases, but are really inconsistent.

So in the case of Ownable, that would probably be an OwnableImpl where we're testing the public-facing behavior of an otherwise production-ready contract.

But if we exposed some internal state to test invariants or wrote a helper function that bypassed authorization logic, that would be a Mock.

So we could either double down on the Mock / Impl distinction or just call everything the same and make a note about it.

[frangio]

The mock terminology is definitely confusing. I think we haven't changed it yet because we haven't come up with a good term. Thanks for bringing it up @fleupold!

The thing is these "mocks" currently serve two different purposes. One of them is, as has been mentioned in the thread, to expose internal APIs. Calling these *Exposed is not a bad idea, but I think I like *Internals better.

The other purpose is to set up an instance in order to then test it. For example this happens with ERC20 because we need to inject some initial balance in order to test its functionality. These definitely are not mocks, except maybe in a contrived sense, and I don't know what would be the right word.

[shrugs]

idea: Testable prefix. aja TestableERC20 or TestableECDSA?