search

duneanalytics / dune-client

A framework for interacting with Dune Analytics' officially supported API service
Apache License 2.0
85 stars 22 forks source link

Remove Redundant Tests #41

Closed bh2smith closed 1 year ago

bh2smith commented 1 year ago

Closes #40 (hopefully)

eliseygusev commented 1 year ago

What is strange about the logs is AttributeError: 'AsyncDuneClient' object has no attribute 'close_session'. Did you mean: '_create_session' because it is now renamed to disconnect (to be consistent with connect) and e.g. https://github.com/cowprotocol/dune-client/blob/main/dune_client/client_async.py#L69 uses it. yet the test with async with fails...

however, i see that i forgot to rename the function in the tests https://github.com/cowprotocol/dune-client/blob/main/tests/e2e/test_async_client.py#L45

might be a reason why it fails on CI and work locally when unit tests are run.

As for the Python version, I think it should be >=3.8.

bh2smith commented 1 year ago

Right - I hadn't pulled main. trying again.

eliseygusev commented 1 year ago

https://github.com/cowprotocol/dune-client/pull/42 Did a PR with fixes

eliseygusev commented 1 year ago

Accidentally closed #42 😅 #43 is the new one without conflicts

bh2smith commented 1 year ago

Alright @eliseygusev - there is something wrong with all of this and I don't have time to keep trying to debug it. I suspect that this client is pinging the URL way more frequently that is specified in the "ping_frequency".

eliseygusev commented 1 year ago

But the failing tests are in sync_client. I think this might be the artifact of running a lot of tests at once (since this PR added some more) and Dune rate limits those after certain point. I guess I can try adding sleeps after the requests, this would make tests run longer but lessen probability of getting "Too many requests"

bh2smith commented 1 year ago

Right! Then maybe I will put some sleep between tests in there. Good point.

bh2smith commented 1 year ago

Actually. I think maybe some of these tests are redundant. Maybe we can just have one async test to demonstrate refresh (which captures almost all the routes anyway. If you don't mind I'm going to cut out some tests from async.

eliseygusev commented 1 year ago

Yeah, thats true. I don't mind cutting some of those as well.