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

Client.OptionsReader() returns a struct ClientOptionsReader instead of interface which make mocking impossible #661

Closed
avmunm opened this issue Dec 14, 2023 · 4 comments

Comments

@avmunm
Copy link
Contributor

avmunm commented Dec 14, 2023

Hi,

Client.OptionsReader returns this object.

// ClientOptionsReader provides an interface for reading ClientOptions after the client has been initialized.
type ClientOptionsReader struct {
	options *ClientOptions
}

When trying to mock the Client and using the options reader in my applications I get a nilpointer, as I can only return an empty struct of ClientOptionsReader. I think it would be better if ClientOptionsReader was an interface, so I could also mock it.
Unless I am missing something else I could do?
Going from this:

func NewMockClient() mqtt.Client {
	return &mockMQTTClient{}
}

type mockMQTTClient struct {
	subscriptions map[string]byte
}

func (c *mockMQTTClient) OptionsReader() mqtt.ClientOptionsReader {
	return mqtt.ClientOptionsReader{}
}

To this:

func NewMockClient() mqtt.Client {
	return &mockMQTTClient{}
}

type mockMQTTClient struct {}

func (c *mockMQTTClient) OptionsReader() mqtt.ClientOptionsReader {
	return &mockOptionsReader
}

type mockOptionsReader struct {}

func (r *mockOptionsReader) ResumeSubs() bool {
	return false
}
....
@MattBrittan
Copy link
Contributor

I've never had the need to use OptionsReader() so this is not something I'd considered and can see your point (there is no way for you to create a valid ClientOptionsReader).

I can see two ways of resolving this:

  • Your suggestion; change the signature of Client such that ClientOptionsReader is an interface.
  • Add a func NewOptionsReader(o *ClientOptions) ClientOptionsReader to the library.

I agree that your solution is the cleanest, however it's also potentially a breaking change, so the second method might be simpler (provides the functionality you need without any potential to break others code).

While the first change is potentially breaking I doubt that many (any?) users are actually using this in a way that would be broken so am open to being convinced that this is the right way to go (but really don't want to have to release a v2 of this library!).

@avmunm
Copy link
Contributor Author

avmunm commented Dec 15, 2023

Thanks for the fast response! If i find some free time I can try to implement a solution and make a PR.
We can see it more clearly once it's implemented I think.

@MattBrittan
Copy link
Contributor

OK - which method where you thinking of going with? (I'd accept a PR utilising the second option without much thought; if you change the ClientOptionsReader then that would require more consideration because it's technically a breaking change).

@avmunm
Copy link
Contributor Author

avmunm commented Dec 15, 2023

I'd chose the second option if you think that's less risky then.
My proposal came from a perspective of what it'd do in my own code base I haven't done any real Library work.

avmunm added a commit to avmunm/paho.mqtt.golang that referenced this issue Dec 19, 2023
avmunm added a commit to avmunm/paho.mqtt.golang that referenced this issue Dec 19, 2023
avmunm added a commit to avmunm/paho.mqtt.golang that referenced this issue Dec 19, 2023
avmunm added a commit to avmunm/paho.mqtt.golang that referenced this issue Dec 19, 2023
avmunm added a commit to avmunm/paho.mqtt.golang that referenced this issue Dec 19, 2023
algitbot pushed a commit to alpinelinux/build-server-status that referenced this issue Sep 16, 2024
This MR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [github.com/eclipse/paho.mqtt.golang](https://github.com/eclipse/paho.mqtt.golang) | require | minor | `v1.4.3` -> `v1.5.0` |

---

### Release Notes

<details>
<summary>eclipse/paho.mqtt.golang (github.com/eclipse/paho.mqtt.golang)</summary>

### [`v1.5.0`](https://github.com/eclipse/paho.mqtt.golang/releases/tag/v1.5.0)

[Compare Source](eclipse-paho/paho.mqtt.golang@v1.4.3...v1.5.0)

In the year since the release of v1.4.3 the majority of changes have been small incremental improvements/fixes. One notable change is that Go v1.20+ is now required (due to MR [#&#8203;646](eclipse-paho/paho.mqtt.golang#646)).

#### What's Changed

-   Wrap connection network errors by [@&#8203;adriansmares](https://github.com/adriansmares) in eclipse-paho/paho.mqtt.golang#646
-   Clarify use of token.WaitTimeout by [@&#8203;MattBrittan](https://github.com/MattBrittan) in eclipse-paho/paho.mqtt.golang#659
-   fix ([#&#8203;661](eclipse-paho/paho.mqtt.golang#661)): Add NewClientOptionsReader for mocking purposes. by [@&#8203;avmunm](https://github.com/avmunm) in eclipse-paho/paho.mqtt.golang#662
-   fix: fix keep-alive timeouts on small intervals by [@&#8203;lefinal](https://github.com/lefinal) in eclipse-paho/paho.mqtt.golang#667
-   Replace the time.After with the timer for efficiency. by [@&#8203;DVasselli](https://github.com/DVasselli) in eclipse-paho/paho.mqtt.golang#671
-   fix: deprecation warnings for ioutil by [@&#8203;vruge](https://github.com/vruge) in eclipse-paho/paho.mqtt.golang#665
-   fix: issue 675:goroutine leak when connectionUp(true) return error by [@&#8203;kiqi007](https://github.com/kiqi007) in eclipse-paho/paho.mqtt.golang#678
-   Update dependencies by [@&#8203;MattBrittan](https://github.com/MattBrittan) in eclipse-paho/paho.mqtt.golang#683

#### New Contributors

-   [@&#8203;adriansmares](https://github.com/adriansmares) made their first contribution in eclipse-paho/paho.mqtt.golang#646
-   [@&#8203;avmunm](https://github.com/avmunm) made their first contribution in eclipse-paho/paho.mqtt.golang#662
-   [@&#8203;lefinal](https://github.com/lefinal) made their first contribution in eclipse-paho/paho.mqtt.golang#667
-   [@&#8203;DVasselli](https://github.com/DVasselli) made their first contribution in eclipse-paho/paho.mqtt.golang#671
-   [@&#8203;vruge](https://github.com/vruge) made their first contribution in eclipse-paho/paho.mqtt.golang#665
-   [@&#8203;kiqi007](https://github.com/kiqi007) made their first contribution in eclipse-paho/paho.mqtt.golang#678

**Full Changelog**: eclipse-paho/paho.mqtt.golang@v1.4.3...v1.5.0

</details>

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

&nbsp;
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC44MC4wIiwidXBkYXRlZEluVmVyIjoiMzguODAuMCIsInRhcmdldEJyYW5jaCI6Im1hc3RlciIsImxhYmVscyI6W119-->

See merge request alpine/infra/build-server-status!15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants