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

Support Actor Protocol #216

Open
tsuzukihashi opened this issue Mar 3, 2023 · 6 comments
Open

Support Actor Protocol #216

tsuzukihashi opened this issue Mar 3, 2023 · 6 comments

Comments

@tsuzukihashi
Copy link

Could you support to actor protocol?
I would like to automatically generate an "Actor" compliant protocol as follows.

/// @mockable
protocol Hoge: Actor {
  func get(hoge: String) async -> Result<String, Error>
  var count: Int { get }
}

↓ Generated

// NOTE: change actor
actor HogeMock: Hoge {
  init() {}
  init(count: Int = 0) {
    self.count = count
  }

  private(set) var getCallCount = 0
  var getArgValues = [String]()
  // NOTE: add @MainActor
  @MainActor
  var getHandler: ((String) async -> (Result<String, Error>))?
  func get(hoge: String) async -> Result<String, Error> {
    getCallCount += 1
    getArgValues.append(hoge)
    // NOTE: add await
    if let getHandler = await getHandler {
      return await getHandler(hoge)
    }
    fatalError("getHandler returns can't have a default value thus its handler must be set")
  }

  private(set) var countSetCallCount = 0
  var count: Int = 0 { didSet { countSetCallCount += 1 } }
  // NOTE: add set func
  func setCount(_ value: Int) {
    self.count = value
  }
}

Thank you.

@uhooi
Copy link
Collaborator

uhooi commented Mar 3, 2023

Thanks for the Issue!
I'm waiting for the PR🥺

@treastrain
Copy link

treastrain commented Mar 3, 2023

I hope this feature exists in this OSS! I see three ways to support this:

  1. Add an annotation specifying the object nominal types of the mock
  2. Mocks for that protocol that conform to the Actor automatically become actor
  3. Mocks for that protocol that conform to the another protocol conforming Actor automatically become actor
// 1.
/// @mockable(object: actor)
protocol Hoge: Actor {
    var count: Int { get }
}

///
/// @Generated by Mockolo
///
actor HogeMock: Hoge {
    init() { }
    init(count: Int = 0) {
        self.count = count
    }

    private(set) var countSetCallCount = 0
    var count: Int = 0 { didSet { countSetCallCount += 1 } }
}
// 2.
/// @mockable
protocol Hoge: Actor {
    var count: Int { get }
}

///
/// @Generated by Mockolo
///
actor HogeMock: Hoge {
    init() { }
    init(count: Int = 0) {
        self.count = count
    }

    private(set) var countSetCallCount = 0
    var count: Int = 0 { didSet { countSetCallCount += 1 } }
}
// 3.
/// @mockable
protocol Hoge: Fuga {
    var count: Int { get }
}
protocol Fuga: Actor {
}

///
/// @Generated by Mockolo
///
actor HogeMock: Hoge {
    init() { }
    init(count: Int = 0) {
        self.count = count
    }

    private(set) var countSetCallCount = 0
    var count: Int = 0 { didSet { countSetCallCount += 1 } }
}

I implemented the changes that fulfill "1." and "2." above in my forked branch: master...treastrain:c20f64b140c78b9f05202e661a07c43c15834358 (No test has been added to this yet now.)

However, I have yet to come up with a way to achieve "3." Any suggestions?

@uhooi
Copy link
Collaborator

uhooi commented Mar 5, 2023

@treastrain Thanks for the suggestion!

I agree with "2." since only actor can conform to the Actor protocol.
Couldn't "3." be achieved using SwiftSyntax and the is operator?
"1." may cause mock build errors when marking protocols that are not Actor.
It seems like it would be better to support only "2." (and "3." if possible) first, what do you think?

Japanese

提案ありがとうございます!

Actor プロトコルに準拠できるのは actor しかないので、"2." に賛成です。
"3." はSwiftSyntaxや is 演算子を使って実現できないでしょうか?

"1." は Actor でないプロトコルにマークした場合にモックがビルドエラーになることがあります。
まずは "2." のみ(できたら "3." も)対応するのがよさそうですがどうでしょうか?

@treastrain
Copy link

@uhooi

Thanks for your response!

The diffs shown in master...treastrain:c20f64b140c78b9f05202e661a07c43c15834358 are a mixture of "1." and "2." but the implementation effort is light < "1." < "2." < heavy.

I agree with "2." since only actor can conform to the Actor protocol.

I see. I will create a pull request for this part in my spare time.

"1." may cause mock build errors when marking protocols that are not Actor.

The same could be said here for the already existing annotations typealias, rx, combine, etc. It is the responsibility of the person making this statement, and we do not believe that we should be concerned about mock build errors when marking protocols that are not Actor.

Couldn't "3." be achieved using SwiftSyntax and the is operator?

My knowledge of SwiftSyntax is lacking, so a solution using it is not immediately obvious.
But as for the is operator, I understand that when this library generates a mock, it does not build and import the source code for the mock generation target, so it can't handle that object directly, so I don't think I can solve it that way.

Japanese ありがとうございます!

master...treastrain:c20f64b140c78b9f05202e661a07c43c15834358 で示した差分は "1." と "2." が混ざったものになっていますが、実装工数は 軽 < "1." < "2." < 重 になっています。

Actor プロトコルに準拠できるのは actor しかないので、"2." に賛成です。

わかりました。私の余暇の時間でここの部分の pull request を作成してみたいと思います。

"1." は Actor でないプロトコルにマークした場合にモックがビルドエラーになることがあります。

こちらは、すでに存在するアノテーションである typealiasrxcombine などでも同じことが言えるかと思います。この記述をする人が責任を持つべき事項であり、「Actor でないプロトコルにマークした場合にモックがビルドエラーになること」をこちら側が心配することではないと思っています。

"3." はSwiftSyntaxや is 演算子を使って実現できないでしょうか?

私の SwiftSyntax に対する知識が足りないため、それを用いた解決方法はすぐには分かりません。
しかし、is 演算子の方については、このライブラリがモックを生成する際に、モック生成対象のソースコードをビルドして import しているわけではないと理解しており、そのオブジェクトを直接取り扱えるわけではないため、その方法ではそれを解決できないと考えます。

@uhooi
Copy link
Collaborator

uhooi commented Mar 6, 2023

@treastrain

I agree with "2." since only actor can conform to the Actor protocol.

I see. I will create a pull request for this part in my spare time.

Thanks, waiting for PR 🙏

"1." may cause mock build errors when marking protocols that are not Actor.

The same could be said here for the already existing annotations typealias, rx, combine, etc. It is the responsibility of the person making this statement, and we do not believe that we should be concerned about mock build errors when marking protocols that are not Actor.

Since you are right, it seems that if "3." cannot be realized by any means, "1." may be adopted.
As for how to realize "3.", the is operator is just an example, and I also thought it could not be solved 😭

Japanese

Actor プロトコルに準拠できるのは actor しかないので、"2." に賛成です。

わかりました。私の余暇の時間でここの部分の pull request を作成してみたいと思います。

ありがとうございます。PR お待ちしております 🙏

"1." は Actor でないプロトコルにマークした場合にモックがビルドエラーになることがあります。

こちらは、すでに存在するアノテーションである typealiasrxcombine などでも同じことが言えるかと思います。> この記述をする人が責任を持つべき事項であり、「Actor でないプロトコルにマークした場合にモックがビルドエラーになること」をこちら側が心配することではないと思っています。

仰る通りなので、どうしても "3." が実現できない場合は "1." を採用してよさそうです。
"3." の実現方法について、 is 演算子は例として挙げただけであり、私も解決できないと思っていました 😭

@fummicc1
Copy link
Collaborator

fummicc1 commented Mar 15, 2023

Thank you! let me say one opinion from my side. 🙏
I prefer option 1 because we can't take care of @globalActor if we adopt option 2. (if it is not correct please teach me.)
Besides that, option 1 is more flexible and independent on certain swift future change.

oh I am sorry. I found globalActor protocol should be mocked with class.

Besides that, option 1 is more flexible and independent on certain swift future change.

one of example is typealias and AnyActor, DistributedActor.

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

No branches or pull requests

4 participants