Skip to content

Fix XMPP plugin#217

Merged
caronc merged 4 commits intocaronc:masterfrom
daq-tools:213-xmpp-improvements
Mar 27, 2020
Merged

Fix XMPP plugin#217
caronc merged 4 commits intocaronc:masterfrom
daq-tools:213-xmpp-improvements

Conversation

@amotl
Copy link
Collaborator

@amotl amotl commented Mar 19, 2020

Dear Chris,

coming from #213, we have been able to make some progress and we are happy with the outcome.

Cheers,
Andreas.

About

This honors sleekxmpp's asynchronous nature by hooking into the
"session_start" event callback. While being at it, some other
things have been addressed.

  • Also hook into the "failed_auth" event callback in order
    to report any authentication failures back to the user.

  • Adjust sleekxmpp's logger to use the logging handlers
    and log level of apprise. That way, "-vvvvv" will yield
    loads of useful information for debugging.

  • Disable presence signalling and roster inquiry.
    Rationale: I believe both XMPP features resonate more
    with human users than bots. Omitting them tremendously
    speeds up processing.

  • xmpp.connect(): Use the "use_ssl=True" argument when the
    effective TCP port equals NotifyXMPP.default_secure_port.

  • xmpp.connect(): Use the "reattempt=False" argument to
    mitigate nasty delay before even connecting to XMPP server.

Heads up

Please note that I had to disable some tests against failures on get_roster's side effects.

@amotl amotl force-pushed the 213-xmpp-improvements branch 2 times, most recently from 057a8b2 to 21aa1af Compare March 19, 2020 02:10
@codecov-io
Copy link

codecov-io commented Mar 19, 2020

Codecov Report

Merging #217 into master will decrease coverage by 0.21%.
The diff coverage is 74.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #217      +/-   ##
==========================================
- Coverage     100%   99.78%   -0.22%     
==========================================
  Files          74       74              
  Lines        8787     8815      +28     
  Branches     1421     1422       +1     
==========================================
+ Hits         8787     8796       +9     
- Misses          0       16      +16     
- Partials        0        3       +3
Impacted Files Coverage Δ
apprise/plugins/NotifyXMPP.py 87.33% <74.19%> (-12.67%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44905e5...4de98f2. Read the comment docs.

@amotl amotl force-pushed the 213-xmpp-improvements branch from 9f8c36e to 1fe751a Compare March 19, 2020 02:50
This honors sleekxmpp's asynchronous nature by hooking into the
"session_start" event callback. While being at it, some other
things have been addressed.

- Also hook into the "failed_auth" event callback in order
  to report any authentication failures back to the user.

- Adjust sleekxmpp's logger to use the logging handlers
  and log level of apprise. That way, "-vvvvv" will yield
  loads of useful information for debugging.

- Disable presence signalling and roster inquiry.
  Rationale: I believe both XMPP features resonate more
  with human users than bots. Omitting them tremendously
  speeds up processing.

- xmpp.connect(): Use the "use_ssl=True" argument when the
  effective TCP port equals NotifyXMPP.default_secure_port.

- xmpp.connect(): Use the "reattempt=False" argument to
  mitigate nasty delay before even connecting to XMPP server.

Resolve caronc#213.
@amotl amotl force-pushed the 213-xmpp-improvements branch from 1fe751a to 3c89b0e Compare March 19, 2020 02:54
@amotl
Copy link
Collaborator Author

amotl commented Mar 19, 2020

Dear Chris,

I am failing on making the tests work again on CI. However, I can confirm they work perfectly on my workbench, both on Python 2.7 and 3.8 and both with or without having the sleekxmpp module actually installed.

You seem to know quite a bunch about the sleekxmpp module camouflage which is going on when ramping up the tests as I recognized some things have been tweaked for CI.

Maybe you will be more lucky than me?

With kind regards,
Andreas.

@amotl
Copy link
Collaborator Author

amotl commented Mar 19, 2020

Can't we just pip install sleekxmpp on CI? That might satisfy the errors.

@caronc
Copy link
Owner

caronc commented Mar 24, 2020

Here is a new patch that brings test coverage up to 92% in the module you refactored (up from 74%):
improved.testing.patch.txt

I still need to figure out how to get sleekxmpp to legitimately call:

  • session_start (which will subsequently call on_before_message)
  • failed_auth

I successfully mocked sleekxmpp.ClientXMPP. I think I/we need to try to find a way of also mocking sleekxmpp.xmlstream.XMLStream and then all of the hooks that are in ClientXMPP can run as normal.

i had a look at the links you sent, but they don't really help because the tests you shared don't actually test that things are working correctly through sleekxmpp (unless i just overlooked them which is very possible 😉)

@amotl
Copy link
Collaborator Author

amotl commented Mar 24, 2020

Dear Chris,

I don't want to distract your path to successful test coverage by hacking your way through the sleekxmpp module. Thanks for making these improvements.

I just wanted to outline that there might be ways to test the sleekxmpp module thoroughly without mocking it at all by either mocking on the socket level (probably tedious) or by spinning up a real XMPP server in the background.

Saying this, I've just discovered that the sleekxmpp module itself has all the machinery to invoke a XMPP server component, see [1].

Cheers,
Andreas.

[1] http://sleekxmpp.com/getting_started/component.html

@caronc
Copy link
Owner

caronc commented Mar 25, 2020

I just wanted to outline that there might be ways to test the sleekxmpp module thoroughly without mocking it at all by either mocking on the socket level (probably tedious) or by spinning up a real XMPP server in the background.

This is definitely a brilliant idea; i spent an hr or too and can't make that tutorial work. 🙁 It doesn't seem to listen on the set port; it also appears to block indefinitely. If you want you an give it a try. I'm about to just give up and mock the functions so the code is at least tested (no typos/exceptions). It's not the best solution but it will achieve the 100% coverage i yearn for (🙂) and more (better) testing can always be added later to improve it.

@amotl
Copy link
Collaborator Author

amotl commented Mar 26, 2020

It doesn't seem to listen on the set port; it also appears to block indefinitely.

Been there, seen that. Please make sure you are using "127.0.0.1" to listen on. Otherwise, it might decide to listen on "::1", thus only being available via IPv6.

@amotl
Copy link
Collaborator Author

amotl commented Mar 26, 2020

Saying this, I've just discovered that the sleekxmpp module itself has all the machinery to invoke a XMPP server component, see [1].

After re-reading the documentation, I discovered this isn't actually a XMPP server component, it is a client component:

Many XMPP applications eventually graduate to requiring to run as a server component in order to meet scalability requirements. To demonstrate how to turn an XMPP client bot into a component, we’ll turn the echobot example (SleekXMPP Quickstart - Echo Bot) into a component version.

@amotl
Copy link
Collaborator Author

amotl commented Mar 26, 2020

Hi Chris,

I've tried to get Prosŏdy through Docker into the mix by a554505. There are some alternatives re. Docker images out there [1,2,3]. However, sleekxmpp is not connecting and things are getting blocked as well.

You can inspect the logs from Docker using docker logs <container-id>, which show something like

$ docker logs 6e578a967b08
startup             info	Hello and welcome to Prosody version 0.11.4
startup             info	Prosody is using the select backend for connection handling
portmanager         info	Activated service 's2s' on [::]:5269, [*]:5269
portmanager         info	Activated service 'c2s' on [::]:5222, [*]:5222
portmanager         info	Activated service 'legacy_ssl' on no ports
c2s55ea6ae61a50     info	Client connected
c2s55ea6ae61a50     info	Client disconnected: connection closed
c2s55ea6ae6b400     info	Client connected
c2s55ea6ae6b400     info	Client disconnected: connection closed
c2s55ea6ae73ef0     info	Client connected
c2s55ea6ae73ef0     info	Client disconnected: connection closed
c2s55ea6ae7d240     info	Client connected

I'm about to just give up and mock the functions so the code is at least tested (no typos/exceptions).

I believe this will be the pragmatic decision. Go ahead and good luck!

With kind regards,
Andreas.

[1] https://hub.docker.com/r/prosody/prosody
[2] https://hub.docker.com/r/opusvl/prosody
[3] https://hub.docker.com/r/unclev/prosody-docker-extended

@caronc caronc mentioned this pull request Mar 26, 2020
4 tasks
@caronc caronc merged commit 7c251bf into caronc:master Mar 27, 2020
caronc added a commit that referenced this pull request Mar 27, 2020
@caronc caronc mentioned this pull request Mar 27, 2020
4 tasks
caronc added a commit that referenced this pull request Mar 27, 2020
caronc added a commit that referenced this pull request Mar 27, 2020
@caronc
Copy link
Owner

caronc commented Mar 27, 2020

Merged what is here for now; but this brought down test coverage below 100%. I have begun trying to complete test coverage (slight re-factoring) in #220. Awesome work @amotl ; thank you. I would actually appreciate any feedback you have on PR referenced. No pressure though! 🙂

@amotl
Copy link
Collaborator Author

amotl commented Mar 28, 2020

Dear Chris,

appreciate it. Thank you very much for picking up my work. Please go ahead and merge #220.

With kind regards,
Andreas.

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.

3 participants