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

[Swift] Generate open classes, methods and mocks #1918

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

nimau
Copy link
Contributor

@nimau nimau commented Dec 28, 2023

This PR is the Swift version of this Kotlin PR, as it makes the concrete type open.

Making the concrete types open allows them to be extended to write mocks, but there is still a lot of code to be written to extend classes before writing a unit test. That's why this PR also includes the ability to generate Swift mocks directly from uniffi-bindgen.

Changes made to generate mocks:

  • a new generate_mocks function is now exposed in library_mode, it uses a new MocksGenerator which is used to render mocks (see wrapper_mocks.swift template)
  • Each mock class inherits from the mocked class (thanks to the open change, this will also work if the mocks are placed in the client application instead of being packaged with the uniffi generated classes).
  • Mocks are written to a special file and are only generated when generate_mocks is called.

Here is an overview of what a mock class that has been generated will look like:

public class TestClassMock: TestClass {

    required public init(unsafeFromRawPointer pointer: UnsafeMutableRawPointer) {
        fatalError("Not supported")
    }

    public init() {
        super.init(noPointer: NoPointer())
    }

    // MARK: - testFunction

    public var testFunctionParameternameCallsCount = 0
    public var testFunctionParameternameCalled: Bool {
        return testFunctionParameternameCallsCount > 0
    }
    public var testFunctionParameternameReceivedInvocations: [String] = []
    public var testFunctionParameternameReceivedArgument: String?
    
    public var testFunctionParameternameThrowableError: Error?
    public var testFunctionParameternameClosure: ((String) throws -> Void)?
    
    /**
     * Test Function documentation
     */
    public override func testFunction(parameterName: String) throws  {
        if let error = testFunctionParameternameThrowableError {
            throw error
        }
        
        testFunctionParameternameReceivedArgument = parameterName
        
        testFunctionParameternameReceivedInvocations.append(parameterName)
        
        testFunctionParameternameCallsCount += 1
        
        if let closure = testFunctionParameternameClosure {
            try closure(parameterName)
        }
    }
}

@nimau nimau requested a review from a team as a code owner December 28, 2023 11:16
@nimau nimau requested review from mhammond and removed request for a team December 28, 2023 11:16
@bendk
Copy link
Contributor

bendk commented Dec 29, 2023

Maybe this could be split into 2:

  • One PR would be to change the classes from public to open. Maybe this should be configurable via uniffi.toml, but it's not critical and could be added later.
  • The other would be to generate the mocks.

The main reason I say this is I wonder if there's a better system for generating the mocks, I wrote up some thoughts in #1920

@bendk bendk mentioned this pull request Dec 29, 2023
@Sajjon
Copy link
Contributor

Sajjon commented Dec 29, 2023

We really want to be able to configure if Swift Classes are open (public) or final. Being able to let classes be final is extremely powerful and important, since otherwise one cannot auto-synthesize Codable conformance.

Here is a mini demo:

Non Final

Broken...
Screenshot 2023-12-29 at 17 58 08

Final

Works!
Screenshot 2023-12-29 at 17 58 14

@stefanceriu
Copy link
Contributor

Being able to let classes be final is extremely powerful and important, since otherwise one cannot auto-synthesize Codable conformance.

While that's useful under normal circumstances I don't think it applies here as extending current uniffi generated classes for Codable results in an Extension outside of file declaring class 'Test' prevents automatic synthesis of 'encode(to:)' for protocol 'Encodable'. How are you currently doing this?

@stefanceriu
Copy link
Contributor

Maybe this could be split into 2:

  • One PR would be to change the classes from public to open. Maybe this should be configurable via uniffi.toml, but it's not critical and could be added later.
  • The other would be to generate the mocks.

The way I understand this, subclasses are useless for anything other than mocking because of the missing backing pointer. As such I don't really see the benefit of splitting the two, especially for nested types in which manually writing mocks would be complicated.

The main reason I say this is I wonder if there's a better system for generating the mocks, I wrote up some thoughts in #1920

That might very well be but I wouldn't block this PR behind what seems to be a larger discussion. If anything having them already in would be a very good test for the new extension system. The most important part in here imho are the mock templates with everything else being easily moved to some other system

@bendk
Copy link
Contributor

bendk commented Jan 3, 2024

The way I understand this, subclasses are useless for anything other than mocking because of the missing backing pointer. As such I don't really see the benefit of splitting the two, especially for nested types in which manually writing mocks would be complicated.

I was thinking that you could manually write them at the start. I could see it getting pretty tedious, but I thought it would be doable. What do you mean by nested types?

That might very well be but I wouldn't block this PR behind what seems to be a larger discussion. If anything having them already in would be a very good test for the new extension system. The most important part in here imho are the mock templates with everything else being easily moved to some other system

I think this goes both ways though. If this PR is tied to a mocking system, then it depends on us agreeing on what a good mocking system is, if UniFFI should be providing one, would we implement mocking in other languages, bikeshedding on the names, etc.

A different way to put it is: what are we committing to by merging this PR? Auto-generating mocks seems like quite a big commitment to me. It would require a lot of discussion and we might not end up landing it. You're suggestion of merging this now with a plan of moving it to be an extension is interesting. I could maybe go for that but I would want more confidence that an extension system was definitely going to happen and we would need to make the plan clear to users.

@stefanceriu
Copy link
Contributor

stefanceriu commented Jan 3, 2024

What do you mean by nested types?

Oh sorry, just meant complex setups where e.g. Client needs Room, Room needs Member, Member needs something else and you end up having to mock the world.

Auto-generating mocks seems like quite a big commitment to me

I agree and I completely understand your point of view but I'm not sure where that leaves us.

Generating mocks at the uniffi layer feels very natural (as the implementation for this PR hopefuly proves) and quite useful for anybody wanting to test downstream. With it hidden behind a different command maybe we don't need a strong commitment for fully fledged extensions or to support all the languages. Might even be good for gauging if other users would be interested in having something similar.

Generating them externally, after the fact, would be the other option but it feels unnecessarily complicated in comparison.

@stefanceriu stefanceriu mentioned this pull request Feb 1, 2024
@mhammond
Copy link
Member

#1975 means we have the open classes/methods, with swift and kotlin parity which is great for tests. So I guess this issue is now just about the mocks?

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.

5 participants