From fc3c57bf67a99eae6029289d4bf363a5a8de8eb9 Mon Sep 17 00:00:00 2001 From: alafanechere Date: Thu, 24 Mar 2022 10:52:29 +0100 Subject: [PATCH 1/3] make update logic clearer --- octavia-cli/octavia_cli/apply/commands.py | 27 ++-- .../unit_tests/test_apply/test_commands.py | 142 +++++++----------- 2 files changed, 73 insertions(+), 96 deletions(-) diff --git a/octavia-cli/octavia_cli/apply/commands.py b/octavia-cli/octavia_cli/apply/commands.py index 76c5dc82dcea..261fcdc30c77 100644 --- a/octavia-cli/octavia_cli/apply/commands.py +++ b/octavia-cli/octavia_cli/apply/commands.py @@ -3,7 +3,7 @@ # from glob import glob -from typing import List, Tuple +from typing import List, Optional, Tuple import airbyte_api_client import click @@ -63,25 +63,27 @@ def apply_single_resource(resource: BaseResource, force: bool) -> None: click.echo("\n".join(messages)) -def should_update_resource(force: bool, diff: str, local_file_changed: bool) -> Tuple[bool, str]: +def should_update_resource(force: bool, user_validation: Optional[bool], local_file_changed: bool) -> Tuple[bool, str]: """Function to decide if the resource needs an update or not. Args: force (bool): Whether force mode is on. - diff (str): The computed diff between local and remote for this resource. + user_validation (bool): User validated the existing changes. local_file_changed (bool): Whether the local file describing the resource was modified. Returns: Tuple[bool, str]: Boolean to know if resource should be updated and string describing the update reason. """ if force: - should_update, update_reason = True, "🚨 - Running update because force mode is on." - elif diff: - should_update, update_reason = True, "✍️ - Running update because a diff was detected between local and remote resource." - elif local_file_changed: + should_update, update_reason = True, "🚨 - Running update because the force mode is activated." + elif user_validation is True: + should_update, update_reason = True, "🟢 - Running update because you validated the changes." + elif user_validation is False: + should_update, update_reason = False, "🔴 - Did not update because you refused the changes." + elif user_validation is None and local_file_changed: should_update, update_reason = ( True, - "✍️ - Running update because a local file change was detected and a secret field might have been edited.", + "🟡 - Running update because a local file change was detected and a secret field might have been edited.", ) else: should_update, update_reason = False, "😴 - Did not update because no change detected." @@ -135,11 +137,14 @@ def update_resource(resource: BaseResource, force: bool) -> List[str]: Returns: List[str]: Post update messages to display to standard output. """ + output_messages = [] diff = resource.get_diff_with_remote_resource() - should_update, update_reason = should_update_resource(force, diff, resource.local_file_changed) - output_messages = [update_reason] + user_validation = None if not force and diff: - should_update = prompt_for_diff_validation(resource.resource_name, diff) + user_validation = prompt_for_diff_validation(resource.resource_name, diff) + should_update, update_reason = should_update_resource(force, user_validation, resource.local_file_changed) + click.echo(update_reason) + if should_update: updated_resource, state = resource.update() output_messages.append( diff --git a/octavia-cli/unit_tests/test_apply/test_commands.py b/octavia-cli/unit_tests/test_apply/test_commands.py index 81708777d79f..6fdd654cad19 100644 --- a/octavia-cli/unit_tests/test_apply/test_commands.py +++ b/octavia-cli/unit_tests/test_apply/test_commands.py @@ -94,47 +94,47 @@ def test_apply_single_resource(patch_click, mocker, resource_was_created): @pytest.mark.parametrize( - "diff,local_file_changed,force,expected_should_update,expected_update_reason", + "force,user_validation,local_file_changed,expect_update,expected_reason", [ - (True, True, True, True, "🚨 - Running update because force mode is on."), # check if force has the top priority - (True, False, True, True, "🚨 - Running update because force mode is on."), # check if force has the top priority - (True, False, False, True, "🚨 - Running update because force mode is on."), # check if force has the top priority - (True, True, False, True, "🚨 - Running update because force mode is on."), # check if force has the top priority + (True, True, True, True, "🚨 - Running update because the force mode is activated."), # check if force has the top priority + (True, False, True, True, "🚨 - Running update because the force mode is activated."), # check if force has the top priority + (True, False, False, True, "🚨 - Running update because the force mode is activated."), # check if force has the top priority + (True, True, False, True, "🚨 - Running update because the force mode is activated."), # check if force has the top priority ( False, True, True, True, - "✍️ - Running update because a diff was detected between local and remote resource.", - ), # check if diff has priority of file changed + "🟢 - Running update because you validated the changes.", + ), # check if user validation has priority over local file change ( False, - True, False, True, - "✍️ - Running update because a diff was detected between local and remote resource.", - ), # check if diff has priority of file changed - ( False, + "🔴 - Did not update because you refused the changes.", + ), # check if user validation has priority over local file change + ( False, + None, True, True, - "✍️ - Running update because a local file change was detected and a secret field might have been edited.", - ), # check if local_file_changed runs even if no diff found + "🟡 - Running update because a local file change was detected and a secret field might have been edited.", + ), # check if local_file_changed runs even if user validation is None. ( False, - False, + None, False, False, "😴 - Did not update because no change detected.", - ), # check if local_file_changed runs even if no diff found + ), # check no update if no local change and user validation is None. ], ) -def test_should_update_resource(patch_click, mocker, diff, local_file_changed, force, expected_should_update, expected_update_reason): - should_update, update_reason = commands.should_update_resource(diff, local_file_changed, force) - assert should_update == expected_should_update +def test_should_update_resource(patch_click, mocker, force, user_validation, local_file_changed, expect_update, expected_reason): + should_update, update_reason = commands.should_update_resource(force, user_validation, local_file_changed) + assert should_update == expect_update assert update_reason == commands.click.style.return_value - commands.click.style.assert_called_with(expected_update_reason, fg="green") + commands.click.style.assert_called_with(expected_reason, fg="green") @pytest.mark.parametrize( @@ -177,80 +177,52 @@ def test_create_resource(patch_click, mocker): @pytest.mark.parametrize( - "force,diff,should_update_resource,expect_prompt,user_validate_diff,expect_update,expected_number_of_messages", + "force,diff,local_file_changed,expect_prompt,user_validation,expect_update", [ - (True, True, True, False, False, True, 3), # Force is on, we have a diff, prompt should not be displayed: we expect update. - ( - True, - False, - True, - False, - False, - True, - 3, - ), # Force is on, no diff, should_update_resource == true, prompt should not be displayed, we expect update. - ( - True, - False, - False, - False, - False, - False, - 1, - ), # Force is on, no diff, should_update_resource == false, prompt should not be displayed, we don't expect update. This scenario should not exists in current implementation as force always trigger update. - ( - False, - True, - True, - True, - True, - True, - 3, - ), # Force is off, we have diff, prompt should be displayed, user validate diff: we expected update. - ( - False, - False, - True, - False, - False, - True, - 3, - ), # Force is off, no diff, should_update_resource == true (in case of file change), prompt should not be displayed, we expect update. - ( - False, - True, - True, - True, - False, - False, - 1, - ), # Force is off, we have a diff but the user does not validate it: we don't expect update. - ( - False, - False, - False, - False, - False, - False, - 1, - ), # Force is off, we have a no diff, should_update_resource == false: we don't expect update. + pytest.param(True, True, True, False, False, True, id="Force, diff, local file change -> no prompt, no validation, expect update."), + pytest.param( + True, True, False, False, False, True, id="Force, diff, no local file change -> no prompt, no validation, expect update." + ), + pytest.param( + True, False, False, False, False, True, id="Force, no diff, no local file change -> no prompt, no validation, expect update." + ), + pytest.param( + True, False, True, False, False, True, id="Force, no diff, local file change -> no prompt, no validation, expect update." + ), + pytest.param( + False, True, True, True, True, True, id="No force, diff, local file change -> expect prompt, validation, expect update." + ), + pytest.param( + False, True, True, True, False, False, id="No force, diff, local file change -> expect prompt, no validation, expect update." + ), + pytest.param( + False, True, False, True, True, True, id="No force, diff, no local file change -> expect prompt, validation, expect update." + ), + pytest.param( + False, True, False, True, False, False, id="No force, diff, no local file change -> expect prompt, no validation, no update." + ), + pytest.param( + False, False, True, False, False, True, id="No force, no diff, local file change -> no prompt, no validation, expect update." + ), + pytest.param( + False, False, False, False, False, False, id="No force, no diff, no local file change -> no prompt, no validation, no update." + ), ], ) -def test_update_resource( - patch_click, mocker, force, diff, should_update_resource, user_validate_diff, expect_prompt, expect_update, expected_number_of_messages -): +def test_update_resource(patch_click, mocker, force, diff, local_file_changed, expect_prompt, user_validation, expect_update): mock_updated_resource = mocker.Mock() mock_state = mocker.Mock() mock_resource = mocker.Mock( get_diff_with_remote_resource=mocker.Mock(return_value=diff), resource_name="my_resource", + local_file_changed=local_file_changed, update=mocker.Mock(return_value=(mock_updated_resource, mock_state)), ) - update_reason = "foo" - mocker.patch.object(commands, "should_update_resource", mocker.Mock(return_value=(should_update_resource, update_reason))) - mocker.patch.object(commands, "prompt_for_diff_validation", mocker.Mock(return_value=user_validate_diff)) + mocker.patch.object(commands, "prompt_for_diff_validation", mocker.Mock(return_value=user_validation)) output_messages = commands.update_resource(mock_resource, force) + commands.click.echo.assert_called_once() + if expect_prompt: commands.prompt_for_diff_validation.assert_called_once_with("my_resource", diff) else: @@ -259,11 +231,9 @@ def test_update_resource( mock_resource.update.assert_called_once() else: mock_resource.update.assert_not_called() - assert output_messages[0] == commands.should_update_resource.return_value[1] == update_reason - assert len(output_messages) == expected_number_of_messages - if expected_number_of_messages == 3: + + if expect_update: assert output_messages == [ - commands.should_update_resource.return_value[1], commands.click.style.return_value, commands.click.style.return_value, ] @@ -273,6 +243,8 @@ def test_update_resource( mocker.call(f"💾 - New state for {mock_updated_resource.name} stored at {mock_state.path}.", fg="yellow"), ] ) + else: + assert output_messages == [] def test_find_local_configuration_files(mocker): From ab33bc6752bc31e9b103db0ee56957546ffc099b Mon Sep 17 00:00:00 2001 From: alafanechere Date: Thu, 24 Mar 2022 11:06:16 +0100 Subject: [PATCH 2/3] use test ids --- .../unit_tests/test_apply/test_commands.py | 51 ++++++++++++++----- 1 file changed, 39 insertions(+), 12 deletions(-) diff --git a/octavia-cli/unit_tests/test_apply/test_commands.py b/octavia-cli/unit_tests/test_apply/test_commands.py index 6fdd654cad19..4a515fec080a 100644 --- a/octavia-cli/unit_tests/test_apply/test_commands.py +++ b/octavia-cli/unit_tests/test_apply/test_commands.py @@ -96,38 +96,65 @@ def test_apply_single_resource(patch_click, mocker, resource_was_created): @pytest.mark.parametrize( "force,user_validation,local_file_changed,expect_update,expected_reason", [ - (True, True, True, True, "🚨 - Running update because the force mode is activated."), # check if force has the top priority - (True, False, True, True, "🚨 - Running update because the force mode is activated."), # check if force has the top priority - (True, False, False, True, "🚨 - Running update because the force mode is activated."), # check if force has the top priority - (True, True, False, True, "🚨 - Running update because the force mode is activated."), # check if force has the top priority - ( + pytest.param( + True, True, True, True, "🚨 - Running update because the force mode is activated.", id="1 - Check if force has the top priority." + ), + pytest.param( + True, + False, + True, + True, + "🚨 - Running update because the force mode is activated.", + id="2 - Check if force has the top priority.", + ), + pytest.param( + True, + False, + False, + True, + "🚨 - Running update because the force mode is activated.", + id="3 - Check if force has the top priority.", + ), + pytest.param( + True, + True, + False, + True, + "🚨 - Running update because the force mode is activated.", + id="4 - Check if force has the top priority.", + ), + pytest.param( False, True, True, True, "🟢 - Running update because you validated the changes.", - ), # check if user validation has priority over local file change - ( + id="Check if user validation has priority over local file change.", + ), + pytest.param( False, False, True, False, "🔴 - Did not update because you refused the changes.", - ), # check if user validation has priority over local file change - ( + id="Check if user validation has priority over local file change.", + ), + pytest.param( False, None, True, True, "🟡 - Running update because a local file change was detected and a secret field might have been edited.", - ), # check if local_file_changed runs even if user validation is None. - ( + id="Check if local_file_changed runs even if user validation is None.", + ), + pytest.param( False, None, False, False, "😴 - Did not update because no change detected.", - ), # check no update if no local change and user validation is None. + id="Check no update if no local change and user validation is None.", + ), ], ) def test_should_update_resource(patch_click, mocker, force, user_validation, local_file_changed, expect_update, expected_reason): From 5c061a60f2fc9985a8a2d3a97921aab9c8ad5bf0 Mon Sep 17 00:00:00 2001 From: alafanechere Date: Fri, 25 Mar 2022 09:08:17 +0100 Subject: [PATCH 3/3] fix typo --- octavia-cli/unit_tests/test_apply/test_commands.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/octavia-cli/unit_tests/test_apply/test_commands.py b/octavia-cli/unit_tests/test_apply/test_commands.py index 4a515fec080a..f216a2e9ad34 100644 --- a/octavia-cli/unit_tests/test_apply/test_commands.py +++ b/octavia-cli/unit_tests/test_apply/test_commands.py @@ -220,7 +220,7 @@ def test_create_resource(patch_click, mocker): False, True, True, True, True, True, id="No force, diff, local file change -> expect prompt, validation, expect update." ), pytest.param( - False, True, True, True, False, False, id="No force, diff, local file change -> expect prompt, no validation, expect update." + False, True, True, True, False, False, id="No force, diff, local file change -> expect prompt, no validation, no update." ), pytest.param( False, True, False, True, True, True, id="No force, diff, no local file change -> expect prompt, validation, expect update."