Refactor to non-recursive solution for target block

[corydickson]

What was wrong?

Issue #19

How was it fixed?

The approach laid out here attempts to reach the target block by measuring the time passed between initial block and the last block given. It is considerably slower than the recursive solution, though addresses the implementation concern. Most likely I'm missing something by not taking advantage of the fact we can look at the block in-between this initial range and search on a smaller set of inputs?

Cute Animal Picture

[njgheorghita]

@corydickson Thanks for the pr! The broken tests are b/c of web3's latest update - I've got a fix coming soon that should solve them

[njgheorghita]

Fix is in - if you rebase against master that should clear up the failing tests

[corydickson]

@njgheorghita Thank you, tho I'm having trouble reproducing the failure in the latest build above to fix

============================================================================================ slowest 10 test durations =============================================================================================
53.84s call     tests/core/test_scraper.py::test_scraper_imports_existing_ethpmcli_dir
41.02s call     tests/core/test_scraper.py::test_scraper_writes_to_disk
26.66s call     tests/core/test_scraper.py::test_get_ethpm_birth_block[4000]
17.98s call     tests/cli/test_registry_cli.py::test_ethpm_registry_commands
13.51s call     tests/cli/test_activate.py::test_activate_etherscan_uri_with_single_deployment
12.91s call     tests/cli/test_install_cli.py::test_ethpm_install_from_etherscan
11.90s call     tests/cli/test_activate.py::test_activate_ipfs_uri_with_factories_and_deployments
10.51s call     tests/cli/test_activate.py::test_activate_registry_uri_with_contract_types_no_deployments
7.12s call     tests/core/test_install.py::test_uninstall_packages[owned-wallet]
6.97s call     tests/core/test_install.py::test_install_multiple_packages
==================================================================================== 120 passed, 1 warning in 627.24s (0:10:27) ==================================================================================== 

[njgheorghita]

I'm a little confused by what's going on here. Wouldn't the min_delta always be delta_end.days?

It also seems that adjusting the hunt for the next block by one is going to take a looong time. I'd try looking at this and see if you can get some better performance by following that pattern.

[corydickson]

You're right, thanks for that resource. I see now that you have to update the range you're looking at for each iteration 😓

[corydickson]

Hope it's okay to add this to the existing test instead of a separate function even though it's no longer looking at only valid intervals

[njgheorghita]

It'd be better if it went in its own test - testing that it raises an exception (a la piper's comment). In most cases, it's better to separate testing concerns into as many functions as necessary rather than squeezing them into a single test (though I'm definitely guilty of this also)

[corydickson]

Thank you for the swift review @njgheorghita ! Let me know if you'd like me to squash the previous commits if things look good

[pipermerriam]

I think this should be an exception case since otherwise it requires checking the return value which is an antipattern since it is easy to forget to do.

[corydickson]

For when the to_block is strictly larger than the from_block

[njgheorghita]

cool! notes like this are definitely worth a comment in the code - and honestly it might be better off as a separate test - then in the test name you can explain what's going on

[corydickson]

Sure, still trying to discern when it's bloat and when it's appropriate for clarity. Any rule of thumbs you use?

[njgheorghita]

Hmmm, imo with tests bloat is not such a big concern, I tend to default to the more tests the better. It's good to have only one specific cause/effect being tested per test. We like to use @pytest.mark.parametrize (example) a lot which helps improve the testing quality while reducing bloat. ping @pipermerriam for thoughts

[njgheorghita]

Thanks! and ughh these cli tests are so flaky - they're working fine locally, so my guess is there's something funky b/w infura requests and circle.

[corydickson]

@pipermerriam @njgheorghita Thank you both learned a lot !