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

Tests/e2e tests staging #1943

Merged
merged 53 commits into from
Jun 13, 2024
Merged

Tests/e2e tests staging #1943

merged 53 commits into from
Jun 13, 2024

Conversation

open-junius
Copy link
Contributor

@open-junius open-junius commented May 27, 2024

The PR will fix #1932.
Some explanation for the implementation.

  1. currently, the output of command includes more lines than expected because of bittensor version. We can see the message like Please update to the latest version at your earliest convenience. Run the following command to upgrade. that why assert len(lines) >= used to check the output.
  2. The root network weight set is disable, so some commands are commented in the test case. Will refactor after new implementation is available in bittensor side.
  3. The test for delegate stake is commented because we can't get the delegate data via subtensor.get_delegates method.

@open-junius open-junius self-assigned this May 27, 2024
@open-junius open-junius marked this pull request as ready for review June 11, 2024 04:22
import bittensor


# we can't test it now since the root network weights can't be set
Copy link

@opendansor opendansor Jun 11, 2024

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?

Copy link
Contributor Author

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.

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.

Copy link
Contributor Author

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(

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?

Copy link
Contributor Author

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

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.

Copy link
Contributor Author

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

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?

Copy link
Contributor Author

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]

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?

Copy link
Contributor Author

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.

Copy link

@opendansor opendansor left a 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.

@open-junius open-junius requested a review from opendansor June 12, 2024 14:07
@open-junius open-junius merged commit d139338 into staging Jun 13, 2024
12 checks passed
@open-junius open-junius deleted the tests/e2e-tests-staging branch June 13, 2024 02:44
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.

5 participants