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

A PR for Issue #25 ( Continuous Integration ) #72

Merged
merged 83 commits into from
Sep 26, 2021
Merged

A PR for Issue #25 ( Continuous Integration ) #72

merged 83 commits into from
Sep 26, 2021

Conversation

JohannSuarez
Copy link
Collaborator

@JohannSuarez JohannSuarez commented Sep 20, 2021

For issue #25

  • Two new workflow files ( .github/workflows/mypy.yml and .github/workflows/pytest.yml )

  • ./ganache.py now has "&" at the end for workflow to run it in the background.

  • Pytest having issues with the config option "collect_ignore" in oceanprotocol/contracts/setup.cfg. I forked the contracts repo to remove the line. For now, the workflow clones my fork of the contracts repo.

  • In tokenspice's README, regarding the instruction about running one test, that specified test doesn't exist anymore. So I edited the README to reference an existing test, and also added the workflow status badges. EDIT: BK's push seemed to have addressed this. I adjusted accordingly.

  • In tokenspice.ini , ARTIFACTS_PATH now points to a user's relative directory instead of the absolute trentmc directory.

@trentmc
Copy link
Contributor

trentmc commented Sep 25, 2021

Lots of progress, cool! Lmk when you would like me to review again. Best,

@JohannSuarez
Copy link
Collaborator Author

Good evening, Trent!

I removed the whitespace, and tokenspice.ini has been modified to your requested changes.
For the failing static type checks, I will need further guidance.

The static type checks have been failing by default even when using MyPy manually outside of this Github workflow. The result of this test is true to the current status of the TokenSPICE repo. I'll have to study more about MyPy and stubs before I can completely tackle the errors. This task could be big enough to be its own separate issue.

In the meantime, we can temporarily hide the MyPy badge from the README to hide the error status, and/or rig the static typechecking workflow to automatically pass.

@trentmc
Copy link
Contributor

trentmc commented Sep 25, 2021

Re typechecks: yes let's hide it for now (as you've done) and then create a new issue for it (as you suggest).

Is the rest ready for review?

@JohannSuarez
Copy link
Collaborator Author

Okay, I have hidden the MyPy badge and set it to always pass.
Yes, I think everything is ready for review

Copy link
Contributor

@trentmc trentmc left a comment

Choose a reason for hiding this comment

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

Most things look good. I run into a few errors when running mypy locally, see below. Could you fix please?

(venv) trentmc@trentmcLnv:~/code/tokenspice$ mypy --config-file mypy.ini ./
assets/agents/DataconsumerAgent.py:25: error: "DataconsumerAgent" has no attribute "_buyDT"
assets/agents/DataconsumerAgent.py:26: error: "DataconsumerAgent" has no attribute "_consumeDT"
engine/test/test_SimStateBase.py:15: error: Method must have at least one argument
engine/test/test_SimEngine.py:19: error: Method must have at least one argument
assets/agents/test/test_MinterAgents.py:5: error: Incompatible import of "AgentBase" (imported name has type Module, local name has type "Type[AgentBase]")
Found 5 errors in 4 files (checked 100 source files)

@JohannSuarez
Copy link
Collaborator Author

Hello, Trent! I have fixed all the errors that have been enumerated above.

Here's my log:

  • engine/test/test_SimStateBase.py - A method had no arguments, so I applied the @staticmethod decorator.

  • engine/test/test_SimEngine.py - A method had no arguments, so I also applied the @staticmethod decorator.

  • assets/agents/DataconsumerAgent.py - There was an if-block that mentioned _buyDT and _consumeDT, neither of which have been defined anywhere, not even in the parent class. A ripgrep search shows these two methods are never mentioned anywhere else in the entire repo. I block commented it out, and left a message explaining why.

  • assets/agents/test/test_MinterAgents.py - MyPy was complaining about the line from engine import AgentBase, which was odd considering dozens of other files have the exact same line. I simple reworded the line to import engine.AgentBase, made adjustments to the lines invoking it, and the error vanished.

These errors were actually what the MyPy workflow kept raising. Upon fixing these, the workflow now passes and so I have unhidden the MyPy badge.

Copy link
Contributor

@trentmc trentmc left a comment

Choose a reason for hiding this comment

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

Looks good. Approving. Then I will merge.

Re DataconsumerAgent::_buyDT - something's up with that. So I just created #80 to fix it. The comment you inserted is appropriate for the meantime.

@trentmc trentmc merged commit 22b784d into tokenspice:main Sep 26, 2021
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