Skip to content

Github Actions implementations to test the agent examples#21

Open
JoseRodrigues443 wants to merge 28 commits into
forta-network:masterfrom
JoseRodrigues443:master
Open

Github Actions implementations to test the agent examples#21
JoseRodrigues443 wants to merge 28 commits into
forta-network:masterfrom
JoseRodrigues443:master

Conversation

@JoseRodrigues443

Copy link
Copy Markdown

Description

Currently, if I or the community wants to add a new example there is no pipeline to guarantee code quality. Also to test if any breaking change on the Forta agent occurred this is a great repo to catch and test that.

So this PR adds a GitHub action that runs a pipeline that builds each agent example and then runs the test battery

An example of this pipeline running can be seen here.

Also, the agent's tests required always a jsonRpcUrl in the forta.config.json config file, making it hard to run the pipeline without any keys or secrets, so was provided a way to test the mocked version without the method getEthersProvider() breaking the build.

Note: The python test currently still require the jsonRpcUrl env variable, because the python fort agent sdk init executes Web3(Web3.HTTPProvider(get_json_rpc_url())) which is not possible to mock for now. In the future I plan to provide a proposal for that fix

Pipeline Image

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Tests and Pipeline
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

All the Javascript and Typescript agents now can run the tests.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@haseebrabbani haseebrabbani left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this is awesome! really appreciate adding these Github actions 🙌🏼

some points though:

  • renaming the agent entry point (i.e. agent.ts -> agentCore.ts) will prevent the agent from running. the CLI looks for a file called agent.js/ts/py to begin execution
  • the default object exported from the agent.js/ts file must provide the handler(s) using the handleTransaction key
  • you bring up a good point about getJsonRpcUrl preventing tests from running without specifying a forta.config.json. I have been planning to set some sort of ENV variable when running tests that would allow this to work

Comment thread README.md Outdated
Minor typo fix
@JoseRodrigues443

JoseRodrigues443 commented Feb 16, 2022

Copy link
Copy Markdown
Author

@haseebrabbani thank you for the fast response 💯

However please take into consideration that the projects still have the agent.(py/js/ts) and seen in the image below.

image

The problem is that the agent file executes the method getEthersProvider() that requires the jsonRpcUrl, with that it became impossible to test the pipeline without putting client secrets on the repo.

What I did to fix that was:

  • agent.(py/js/ts) still exists and still have the handleBlock method available so the CLI continues to work
  • Created a agentCore that haves the provideHandleBlock logic that allows testing the core logic without having the jsonRpcUrl set

this is awesome! really appreciate adding these Github actions 🙌🏼

some points though:

  • renaming the agent entry point (i.e. agent.ts -> agentCore.ts) will prevent the agent from running. the CLI looks for a file called agent.js/ts/py to begin execution
  • the default object exported from the agent.js/ts file must provide the handler(s) using the handleTransaction key
  • you bring up a good point about getJsonRpcUrl preventing tests from running without specifying a forta.config.json. I have been planning to set some sort of ENV variable when running tests that would allow this to work

@haseebrabbani

Copy link
Copy Markdown
Collaborator

hey @JoseRodrigues443 🙂 I just released JS SDK v0.0.39 and Python SDK v0.0.13 which fixes the issue of getJsonRpcUrl throwing an error when running unit tests. I have updated the problem examples (flash-loan-ts, minimum-balance-py, minimum-balance-ts) so you shouldn't need to split out those agent files anymore. Would you mind integrating with the latest changes and trying to run the build? I would love to merge this PR 🙌🏼

* Merge

* Agent core separation is not needed anymore

* Added generic test

* Added ignore missing test

* Added ignore py
@JoseRodrigues443

Copy link
Copy Markdown
Author

Fixed @haseebrabbani

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.

2 participants