-
Notifications
You must be signed in to change notification settings - Fork 358
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
Tests/e2e tests staging #1943
Tests/e2e tests staging #1943
Conversation
import bittensor | ||
|
||
|
||
# we can't test it now since the root network weights can't be set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we cant test this right now, should we remove it until we can properly test it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls keep it in the repo. I need some time to try, then know what's the correct parameter and how to check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you keep this in a separate branch? There is no need to push this in while its still under development and not ready to go out to the public. Every test we run on the Bittensor side actually tests what it is supposed to test. This will add tests that don't actually test what they claim to test, and we will be required to "remember" that X or Y test is incomplete, giving us holes in what bugs and issues can go unnoticed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I will remove it.
assert delegates == 11796 | ||
|
||
# stake 1 TAO | ||
# exec_command( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of these commented out blocks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to add and reduce stake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand, but the code is commented out... Meaning the test doesn't actually add or reduce stake, and test that stake is added or reduced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, will just keep it in my local branch
from ...utils import new_wallet, sudo_call_set_network_limit | ||
|
||
|
||
# we can't test it now since the root network weights can't be set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this test run correctly? Maybe we write this test when the command is updated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extrinsic in subtensor side is updated. From @distributedstatemachine , I know cortex team will change accordingly.
assert len(lines) >= 4 | ||
bittensor.logging.info(lines) | ||
|
||
# assert "Root Network" in lines[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these lines commented out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned in the description, we always get the extra output about bittensor version. so just commented out, can add them back after version is aligned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add tests that actually test what they claim to test. If X or Y test is still under development, keep it in a git branch and work on it as you see fit, but there is no need to push in tests that don't accomplish what they set out to do.
The PR will fix #1932.
Some explanation for the implementation.
Please update to the latest version at your earliest convenience. Run the following command to upgrade
. that whyassert len(lines) >=
used to check the output.