Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fixed tests and logic to find the fastest endpoint #33

Merged
merged 45 commits into from
Jul 31, 2024

Conversation

gentlementlegen
Copy link
Member

Resolves #32

Copy link
Contributor

Unlisted binaries (1)

Filename binaries
.github/workflows/publish-package.yml publish

@gentlementlegen
Copy link
Member Author

Seems the latest run that didn't have the issue was https://github.com/ubiquity/rpc-handler/actions/runs/10122809912
Will have a look.

@Keyrxng
Copy link
Contributor

Keyrxng commented Jul 29, 2024

good luck, it's got hands

tests/benchmark.test.ts Outdated Show resolved Hide resolved
@gentlementlegen
Copy link
Member Author

gentlementlegen commented Jul 30, 2024

@Keyrxng I solved the testing issue by mocking the post from axios because we do not need to hit the real endpoints anyway. Also I removed the tests where you were testing JsonRpcProviders because you are testing their API not ours, so I don't think that was really relevant. Everything is green now.

tests/call-handler.test.ts Outdated Show resolved Hide resolved
@Keyrxng
Copy link
Contributor

Keyrxng commented Jul 30, 2024

Also I removed the tests where you were testing JsonRpcProviders because you are testing their API not ours

I agree the first/top two tests were testing the provider API endpoints but only with read ops but the following write ops were all targeting the local anvil instance.

  const mods: HandlerConstructorConfig = {
      runtimeRpcs: rpcList.map((rpc) => rpc.url),
      networkRpcs: rpcList,
      autoStorage: false,
      cacheRefreshCycles: 1,
      networkName: "anvil",
      networkId: "31337",
      rpcTimeout: 700,
      proxySettings: {
        retryCount: 1,
        retryDelay: 10,
        logTier: "verbose",
        logger: new PrettyLogs(),
        strictLogs: false,
      },
    };

The provider racing was the original functionality of this package but it's now in control of all read and write operations so I think we should have tests for this

@gentlementlegen
Copy link
Member Author

@Keyrxng I did my best to add back the tests with mods and naming them coherently, let me know what you think.

Copy link
Contributor

@Keyrxng Keyrxng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rpc-handler.test.ts contains tests related to the original functionality of the package, so things like proper init values and it returning the fastest provider.

call-handler.test.ts should contain tests that are testing the proxy and therefor all of the retry and reselection logic

As far as I can see all of the call-handler.test tests are now testing with expect(provider).toBeTruthy(); which is only testing for the successful returning of a provider on init

Checkout this jest run as I merged proxify. No leaking worker and lots of logs showing that the retry logic was being hit and was working. This latest run only shows provider initialized logs and doesn't appear to be hitting the retry logic

@gentlementlegen
Copy link
Member Author

@Keyrxng Right ok I get it. But then I should try to entirely mock the RPC provider logic because we do not want real endpoints to be hit.

@Keyrxng
Copy link
Contributor

Keyrxng commented Jul 30, 2024

@Keyrxng Right ok I get it. But then I should try to entirely mock the RPC provider logic because we do not want real endpoints to be hit.

Well no actually because anvil is dumb and will respond successfully more or less no matter what you send to it, so in the specific case that our nonceBitMap read op was failing and it was only for certain providers anvil will always give you a great light under that scenario

Maybe I am mistaken @gentlementlegen and you can do that it may be possible so let me correct above and just say that the above is my opinion and that's why I took the approach that I did but you can mock everything if we are still able to effectively test the call-handler aspect I guess

@gentlementlegen
Copy link
Member Author

@Keyrxng Took another different approach: I removed the mocks and always use Anvil instead, tell me what you think.

Copy link
Contributor

@Keyrxng Keyrxng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good as far as I can tell. Test logs showing retry logic and no leaky worker error

@gentlementlegen gentlementlegen merged commit e50fa28 into ubiquity:development Jul 31, 2024
3 checks passed
@gentlementlegen gentlementlegen deleted the fix/tests branch July 31, 2024 08:10
@ubiquibot ubiquibot bot mentioned this pull request Jul 31, 2024
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.

Fix RPC tests
3 participants