wechaty / puppet-mock

Puppet Mocker for Wechaty (& A Puppet Template Starter)
https://paka.dev/npm/wechaty-puppet-mock
Apache License 2.0
45 stars 14 forks source link

Support other message type. #40

Open chs97 opened 3 years ago

chs97 commented 3 years ago

Problem

When I use a mocker to test my robot, I want to send some other type to my robot, such as UrlLink、Filebox, because my robot will not reply message except text type. My test code like:


  const mp = new MiniProgram({
    title: 'mock',
  })
  await puppet.start()
  user.say(mp).to(mary)

I find the mocker only supports the text.

Solution

It's mock, all object can set or get on memory, so I think to use a map make other type binds to the message. Make the API the same as Message. It's very clear that when a message type isn't text, then get the attachment from the pool.

39

huan commented 3 years ago

Thank you very much for the explanation for the design of your PR! It's a great improvement for the mocker to support the attachments.

The design & code are good to make things work, and I'm thinking about the following two improvements:

  1. Design a better API for mocker to manage the attachment, for example:
    const att = mocker.createAttachment(...)
    player.say(att).to(bot)
  2. Make the attachment manager at the mocker level, instead of the message level.

However, I believe we can refactor the code in the future, and we are good for merging your PR now.

Cheers!

huan commented 3 years ago

The PR has been merged, and the new version of wechaty-puppet-mock should be published to NPM after a while.

I'd like to invite you to join our Wechaty Org on GitHub, please accept it by following the below message:

You've invited chs97 to Wechaty! They'll be receiving an email shortly. They can also visit https://github.com/wechaty to accept the invitation.

And we have a WeChaty Contributors WeChat room for contributors only, if you'd like to join, please contact @lijiarui .

Thank you again for your contribution, and welcome to join our great Wechaty Contributors!

chs97 commented 3 years ago

Thank you very much for the explanation for the design of your PR! It's a great improvement for the mocker to support the attachments.

The design & code are good to make things work, and I'm thinking about the following two improvements:

  1. Design a better API for mocker to manage the attachment, for example:
    const att = mocker.createAttachment(...)
    player.say(att).to(bot)
  2. Make the attachment manager at the mocker level, instead of the message level.

However, I believe we can refactor the code in the future, and we are good for merging your PR now.

Cheers!

create from mocker but bind from the message? I think the attachment should bind with the message. The message is bound to at most one attachment. I prefer to make the same API as normal.

const URL = new UrlLink()
player.say(URL).to(bot)
// or
message.say(URL)
huan commented 3 years ago

I prefer to make the same API as normal.

Your proposal looks great! Please feel free to implement it and send PR to the source.

huan commented 3 years ago

Hello @chs97,

Today I have to remove the attachment support related code and unit tests from the master branch because the current design has a big problem: it has a dependency on wechaty, which it should not.

The reason that we can not have a dependency on wechaty is that the wechaty has a dependency on wechaty-puppet-mock. If the wechaty-puppet-mock dependent on wechaty too, there will cause problems.

Your code will remain in wechaty-puppet-mock@0.25, so if you need them you can specify version 0.25 and you will get everything you need.

My latest change will take effect from wechaty-puppet-mock@0.27.

P.S. It will be great if you'd like to continue to discuss how to add attachments support to our mockers because it's a necessary feature that should be implemented.

chs97 commented 3 years ago

@huan only attachment type depends on wechaty, some way to fix it.

  1. set attachment type to Any Type, not check type when extra attachment from mockMessage.
  2. Move out FileBox、UrlLink... from wechaty.
  3. set attachment type to Any Type, use other helper function to check attachment type on runtime, make sure contact can't say something exclude FileBox、UrlLink...
chs97 commented 3 years ago

There is a discussion about this in the WeChat group. (2020.07.26, 11:11 am)

Question

there is a cyclic dependency problem. wechaty dependency wechaty-puppet-mock and wechaty-puppet-mock dependency wechaty. When developing wechaty, wechaty-puppet-mock can't install dependencies actually, will throw an error of dependence not found.

Solution

  1. Extract UrlLink FileBox... from wechaty and package them into wechaty-common (Form Yangfan)
  2. Build MockUrlLink MockFileBox... on the mock system, make their API the same as wechaty.And check them use payload in your test case.Such as :
    const urlLink = new MockUrlLink()
    wechaty.on('message', message => {
    const receive = await message.toUrlLink()
    check(receive.payload, urlLink.payload)
    })
    mock.say(urlLink).to(wechaty)

Conclusion

The purpose is to remove wechaty dependency. make the test case easier to write.

wechaty-puppet-mock has unclear boundaries, Its responsibilities are not only part wechaty but also part wechaty-puppet. In principle, wechaty-peppet-* only transfer data., no instantiation process. For example: On the mock system, we should consider the local files first, not remote. there are only metadata on the mock system, like title, type... FileBox load remote file logic can test alone.

My English is very poor, you can edit this comment

huan commented 3 years ago

@chs97 Thank you very much for summarising our conversation from WeChat contributors room!

The following are my two comments:

UrlLink

Extract UrlLink FileBox... from wechaty and package them into wechaty-common (Form Yangfan)

The FileBox should be ok because it is already a core component of our wechaty-puppet.

The UrlLink has already had a payload in our wechaty-puppet at https://github.com/wechaty/wechaty-puppet/blob/master/src/schemas/url-link.ts which we can reuse it in our mock system.

Boundary

wechaty-puppet-mock has unclear boundaries, Its responsibilities are not only part wechaty but also part wechaty-puppet.

I do not agree with the above comments.

The wechaty-puppet-mock has a very clear boundary:

  1. It is a child class of wechaty-puppet (Puppet) instead of part of it.
  2. It is a dependence of wechaty
huan commented 4 months ago

Hi @chs97

Happy New Year 2024!

If you are still interested in Wechaty testing, you are welcome to join Wechaty Discord https://discord.gg/7q8NBZbQzt for future Wechaty Puppet Mock testing discussions. Please DM me after you join.

I have a plan to improve this module in 2024.

Have a great day!