Fix Name Override With Inheritance

[plx]

This addresses current limitations mockolo from working as expected when mocking a protocol whose parent protocol that has a custom name override, e.g. this situation:

// In BaseProtocol.swift
/// @mockable(override: name = FakeBase)
protocol BaseProtocol {
    func register()
    var counter: Int { get }
}

// In DerivedProtocol.swift, will be a separate mock invocation
/// @mockable()
protocol DerivedProtocol: BaseProtocol {
    func like()
    func subscribe()
}

In such cases, mockolo currently fails to recognize FakeBase as the parent mock for DerivedProtocol, and thus fails to include the generated implementation from FakeBase in the body of DerivedProtocol.

The changes in this PR correct that.

All existing unit tests still pass, and I added new unit tests covering the behavior above.

The essential part of the change is explicitly maintaining a map from protocol names to the names of their mocks; it also addresses a few other places where the code implicitly only worked with the default naming pattern.

[CLAassistant]

All committers have signed the CLA.

[sidepelican]

Thanks for the fixes! You’re really diving into the "abyss" of Parent Mock logic.

I’m still considering the necessity of inheritanceByProtocolMap, but I couldn't fully understand its purpose from the added tests.

Could we start by refactoring the tests first?

[sidepelican]

The classesHaveBeenProcessed flag might look complex at first, but it makes perfect sense.
By using this flag, I think we can remove unreliable checks like suffix matching.

Suggested change

	if classesHaveBeenProcessed || node.nameText.hasSuffix("Mock") {
	if classesHaveBeenProcessed {

[sidepelican]

Since this flag originates from the mockfiles option, maybe a name like scanAsMockfile would be better?

[plx]

Yes, I agree—changed in incoming updates.

[plx]

I updated the tests to explain the behavior they're trying to verify, but that might not be enough to explain the problem motivating the addition of inheritanceByProtocolMap.

The backstory here is working on a codebase like this:

• we have a large, highly-modularized codebase
• "modules" are split into an interface and an implementation:
  • interfaces are "public", and only expose protocols and module-specific basic data types
  • implementations have the concrete implementations, and are only for use in the actual app (e.g. to construct the dependencies that get injected)
• we use @mockolo to auto-generate mocks as much as possible (because...it's awesome)

The problem we were hitting is that:

• we have a fair amount of cross-module protocol inheritance, like this:
  • XYZFeatureProtocol is in the XYZFeature module's XYZFeatureInterface target
  • XYZFeatureProtocol inherits from AbstractFeatureProtocol (which is in a separate, core target)
• we have pre-existing naming conventions that aren't the same as mockolo:
  • mockolo wants XYZFeatureProtocolMock
  • our convention is XYZFeatureMock

As far as we could determine, mockolo couldn't handle this exact combination, and specifically b/c when generating the mock for XYZFeatureProtocol it'd be looking for AbstractFeatureProtocolMock (the default) and not AbstractFeatureMock (our convention).

The inheritanceByProtocolMap exists to track "this $protocol has been mocked as $mockName, so mockolo` can fully match things up during generation. I have also added a review comment showing where we added the logic to make that connection happen.

[plx]

Just as a note, I left this line unchanged b/c both the original tests and the ones I added work without the change. It sill looks like something that might cause an issue like I was trying to fix, but if so I never found an invocation that'd trigger it.

Just pointing it out since you have more context on the overall codebase.

[sidepelican]

To be honest, I'm not particularly familiar around this codebase.

I believe using the Mock suffix for identification purposes isn't ideal.
However, since class inheritance considerations sometimes require relying on suffixes, leaving it unchanged seems appropriate.

[sidepelican]

The changes have made the intent much clearer. Thank you !

The existing implementation identifies Parent Mock by class name,
but your changes add logic to identify Parent Mock using the protocol name it's inheriting.

I think this makes perfect sense logically, and I appreciate the consideration for backward compatibility.

[sidepelican]

To be honest, I'm not particularly familiar around this codebase.

I believe using the Mock suffix for identification purposes isn't ideal.
However, since class inheritance considerations sometimes require relying on suffixes, leaving it unchanged seems appropriate.