-
Notifications
You must be signed in to change notification settings - Fork 76
Conversation
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.
Looking good! Left some comments
tests/commands/test_node.py
Outdated
([HOSTS[1], PORTS[1]], HOSTS[1], PORTS[1], None), | ||
([HOSTS[1], PORTS[1], LITE], HOSTS[1], PORTS[1], LITE), |
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’m not sure why these two lines use HOSTS[1]
, PORTS[1]
, and LITE
as args. i guess they’re just placeholder values but if that's the case then it causes a bit of confusion. or do they serve a different purpose?
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.
It's to make sure that when these args are explicitly passed, it doesn't cause any issues with the command (like it did with --account_contract
and disable_hint_validation
in #150). I'll refactor to make it less confusing
tests/commands/test_node.py
Outdated
|
||
command = ["starknet-devnet", "--host", host, "--port", port] | ||
if mode: | ||
command.append(mode) | ||
mock_subprocess.assert_called_once_with(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.
aren't we testing args
?
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.
Yes! I think the weirdness of this test comes from checking the defaults (which differ from args except when explicitly set)
Co-authored-by: Martín Triay <[email protected]>
…ite-mode' into add-lite-mode
This PR supercedes #144 and resolves #116.