uperl / AnyEvent-WebSocket-Client

WebSocket client for AnyEvent
9 stars 10 forks source link

Tests fail (with Protocol::WebSocket 0.22?) #33

Closed eserte closed 6 years ago

eserte commented 6 years ago

Tests started to fail on my smokers:

        # Failed test at t/anyevent_websocket_connection.t line 154.
        # Unicode::GCString is not installed, table may not display all unicode characters properly
        # +---------------------+---------------------+----+----------+----------+
        # | PATH                | GOT                 | OP | CHECK    | LNs      |
        # +---------------------+---------------------+----+----------+----------+
        # |                     | AnyEvent::WebSocket |    | <OBJECT> | 150, 153 |
        # |                     | ::Connection=HASH(0 |    |          |          |
        # |                     | x44ca5cf0)          |    |          |          |
        # |                     |                     |    |          |          |
        # | close_code()        | 963                 | eq | 1005     | 440      |
        # | close_reason()      | �bb                 | eq | bb       | 440      |
        # +---------------------+---------------------+----+----------+----------+
    # Failed test 'close'
    # at t/anyevent_websocket_connection.t line 155.

(etc)

Statistical analysis suggests that the problem started with Protocol::WebSocket 0.22 (@vti @leonerd: FYI):

****************************************************************
Regression 'mod:Protocol::WebSocket'
****************************************************************
Name                   Theta          StdErr     T-stat
[0='const']           1.0000          0.0000    63928868152459800.00
[1='eq_0.21']         0.0000          0.0000       2.74
[2='eq_0.22']        -1.0000          0.0000    -40146423047850984.00

R^2= 1.000, N= 135, K= 3
****************************************************************
plicease commented 6 years ago

Indeed, these changes are likely the trigger: https://metacpan.org/diff/file?target=VTI/Protocol-WebSocket-0.22/lib%2FProtocol%2FWebSocket%2FFrame.pm&source=VTI%2FProtocol-WebSocket-0.21%2Flib%2FProtocol%2FWebSocket%2FFrame.pm

vti commented 6 years ago

Sorry, blame @leonerd for that :)

leonerd commented 6 years ago

Oops. This is almost certainly related to Unicode string vs. UTF-8 character handling in the surrounding code.

If someone can give me a bit of hand-holding about development/setup/testing on this code I'll be happy to take a look and see if I can fix it.

leonerd commented 6 years ago

Oh actually it's a ~2line patch to fix.

GH33.patch.txt

You may need to bump the version requirement of Protocol::WebSocket to the earliest one in which Protocol::WebSocket::Frame supports the opcode constructor argument - offhand I don't know which one that is. Maybe @vti can assist?

Clarification: It shouldn't need Protocol::WebSocket 0.22 itself here, as the change should be back-compat. The only issue was setting opcode before parsing the buffer, as it now needs to know if it's a text frame or not before it does that.

plicease commented 6 years ago

Thanks for the speedy fix. We already require Protocol::WebSocket 0.20, is that sufficient?

plicease commented 6 years ago

Patch seems to work with 0.20, 0.21, 0.22 and 0.23. Since the change was reverted in 0.23 it isn't necessary there (?), but it doesn't seem to do any harm either.

vti commented 6 years ago

It effected other modules that's why I reverted it for now.

plicease commented 6 years ago

yeah. I gathered.

I've released this fix as WSC 0.44, as it fixes compat with PW 0.22 and doesn't seem to break any of the other versions that we support. Please let me know if there are any changes in WSC required for good fix referred to here: https://github.com/vti/protocol-websocket/issues/54#issuecomment-351162385

vti commented 6 years ago

I will try to make sure dependent modules keep working %)

On Dec 12, 2017 21:43, "✈ Graham ✈" notifications@github.com wrote:

yeah. I gathered.

I've released this fix as WSC 0.44, as it fixes compat with PW 0.22 and doesn't seem to break any of the other versions that we support. Please let me know if there are any changes in WSC required for good fix referred to here: vti/protocol-websocket#54 (comment) https://github.com/vti/protocol-websocket/issues/54#issuecomment-351162385

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/plicease/AnyEvent-WebSocket-Client/issues/33#issuecomment-351188124, or mute the thread https://github.com/notifications/unsubscribe-auth/AAB6M4tEvilnGTOMYGBAfhFDkouUCsHNks5s_uV4gaJpZM4Q8Zzr .