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

Fixed the yes/no prompt in verdi computer delete #6624

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/aiida/cmdline/commands/cmd_computer.py
Original file line number Diff line number Diff line change
Expand Up @@ -634,7 +634,7 @@ def _dry_run_callback(pks):
if pks:
echo.echo_report('The nodes with the following pks would be deleted: ' + ' '.join(map(str, pks)))
echo.echo_warning(f'YOU ARE ABOUT TO DELETE {len(pks)} NODES! THIS CANNOT BE UNDONE!')
confirm = click.prompt('Shall I continue? [yes/N]', type=str) == 'yes'
confirm = click.confirm('Shall I continue?', default=False)
if not confirm:
raise click.Abort
return not confirm
Copy link
Contributor

Choose a reason for hiding this comment

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

May I take the chance to improve the message at line 642:
(if there are no nodes, this is a bit annoying saying "0 nodes marked for deletion")

if node_pks: delete_nodes(node_pks, dry_run=dry_run or _dry_run_callback)

Expand Down
79 changes: 39 additions & 40 deletions tests/cmdline/commands/test_computer.py
Original file line number Diff line number Diff line change
Expand Up @@ -772,76 +772,75 @@ def test_computer_relabel(self):
# The new label should be available
orm.Computer.collection.get(label='comp_cli_test_computer')

def test_computer_delete_with_nodes(self):
"""check if 'verdi computer delete' works when there are associated nodes"""
# ensure that 'yes' works for backwards compatibility, see issue #6422
@pytest.mark.parametrize('user_input', ['y', 'yes'])
def test_computer_delete_existing_computer_successful(self, user_input):
"""Test if `verdi computer delete` works correctly for an existing computer when it is deleted"""
from aiida.common.exceptions import NotExistent

label = 'computer_69'
compute_temp = orm.Computer(
label = 'computer_temp'
computer_temp = orm.Computer(
label=label,
hostname='localhost',
transport_type='core.local',
scheduler_type='core.direct',
workdir='/tmp/aiida',
)
compute_temp.store()
compute_temp.configure(safe_interval=0)
computer_temp.store()
computer_temp.configure(safe_interval=0)

c_label = 'code_69'
code_label = 'code_temp'
orm.InstalledCode(
label=c_label,
label=code_label,
default_calc_job_plugin='core.arithmetic.add',
computer=compute_temp,
computer=computer_temp,
filepath_executable='/remote/abs/path',
).store()

false_user_input = 'y' # most common mistake
user_input = 'yes'

# Abort in case of wrong input
self.cli_runner(computer_delete, [label], user_input=false_user_input, raises=True)
orm.load_code(c_label)

# Safety check in case of --dry-run
options = [label, '--dry-run']
self.cli_runner(computer_delete, options)
orm.load_code(c_label)

# A successul delete, including all associated nodes
self.cli_runner(computer_delete, [label], user_input=user_input)

with pytest.raises(NotExistent):
orm.Computer.collection.get(label=label)

with pytest.raises(NotExistent):
orm.load_code(c_label)

def test_computer_delete(self):
"""Test if 'verdi computer delete' command works"""
from aiida.common.exceptions import NotExistent
orm.load_code(code_label)

# Setup a computer to delete during the test
label = 'computer_for_test_label'
orm.Computer(
def test_computer_delete_existing_computer(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

I found this test name kinda misleading for it's purpose

Suggested change
def test_computer_delete_existing_computer(self):
def test_computer_do_not_delete(self):

"""Test if `verdi computer delete` works correctly for an existing computer when it is not deleted"""
label = 'computer_temp'
computer_temp = orm.Computer(
label=label,
hostname='localhost',
transport_type='core.local',
scheduler_type='core.direct',
workdir='/tmp/aiida',
)
computer_temp.store()
computer_temp.configure(safe_interval=0)

code_label = 'code_temp'
orm.InstalledCode(
label=code_label,
default_calc_job_plugin='core.arithmetic.add',
computer=computer_temp,
filepath_executable='/remote/abs/path',
).store()
# and configure it
options = ['core.local', label, '--non-interactive', '--safe-interval', '0']
self.cli_runner(computer_configure, options)

# See if the command complains about not getting an invalid computer
user_input = 'yes'
self.cli_runner(computer_delete, ['computer_that_does_not_exist'], raises=True, user_input=user_input)
# Tests that Abort in case of wrong input
false_user_input = '' # most common input that could happen by accident
self.cli_runner(computer_delete, [label], user_input=false_user_input, raises=True)
orm.load_code(code_label)

# Delete a computer name successully.
self.cli_runner(computer_delete, [label], user_input=user_input)
# Check that the computer really was deleted
with pytest.raises(NotExistent):
orm.Computer.collection.get(label=label)
# Safety check in case of --dry-run
options = [label, '--dry-run']
self.cli_runner(computer_delete, options)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.cli_runner(computer_delete, options)
self.cli_runner(computer_delete, options, user_input="y")

orm.load_code(code_label)

def test_computer_delete_nonexisting_computer(self):
"""Test if `verdi computer delete` command works correctly for a nonexisting computer"""
# See if the command complains about not getting an nonexisting computer
self.cli_runner(computer_delete, ['computer_that_does_not_exist'], raises=True)
Copy link
Contributor

@khsrali khsrali Nov 28, 2024

Choose a reason for hiding this comment

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

Please check explicitly if the raise is due to NotExistent.
Also please pass user_input=y I know here it might not matter because the query comes before click.confirm,
however, later if someone changes the order of the logic in the code then test might silently pass..
Better to be precise.
Thanks



@pytest.mark.parametrize('non_interactive_editor', ('vim -cwq',), indirect=True)
Expand Down
Loading