search

nspcc-dev / neo-go

Go Node and SDK for the Neo blockchain
MIT License
123 stars 79 forks source link

Notification subscription filter unit test checks nothing #3693

Open carpawell opened 2 days ago

carpawell commented 2 days ago

https://github.com/nspcc-dev/neo-go/blob/57eec71101bad9d6d01bbaeecba0a2105f9dbd6b/pkg/services/rpcsrv/subscription_test.go#L244-L263

Current Behavior

Subscription filters (execution notification) are called, none of the events are ever sent back, check funcs are also never called, test does nothing.

Expected Behavior

Use some real notifications that are triggered and that can be tested. E.e. from the Rubl contract or from some native contract.

Possible Solution

Drop my_pretty_notification, this name is from nowhere. I don't know if it ever worked before but now it does not for sure. Use some real notifications from dumped blocks.

Steps to Reproduce

Add some print in check func, see that it is never called or do my_pretty_notification search, see that it is never called, does not exist in any example contract, and is not passed in any predefined-blocks code, this event cannot happen from nowhere.

Context

I tried to understand how to extend notification filter tests and could not understand why it works but it just does not work.

Regression

Not sure, seems like not, the commit that adds it, does not change any example contract code.

Your Environment

v0.106.3

carpawell commented 2 days ago

Also, I cannot understand why this code works but seems like it works: https://github.com/nspcc-dev/neo-go/blob/57eec71101bad9d6d01bbaeecba0a2105f9dbd6b/pkg/services/rpcsrv/subscription_test.go#L86

This is a buffer for server messages, but why is it 16-messages long? We have at least 23 blocks and every test subscribes for blocks first and then reads subscriptions. One routine, block by block. It cannot fit the buffer and I think it should lock somehow but tests work ok (or just pretend to work ok). I am afraid at some point this test can lock and it would take some time to understand what is happening. What am I missing? Is everything ok with this code? It has not changed since the notification release.