From 130b25c57a0b9d2506bce25b140faf756f8e3162 Mon Sep 17 00:00:00 2001 From: Jake Beresford Date: Wed, 23 Nov 2022 11:56:50 -0500 Subject: [PATCH 1/2] Fix issue with plugin.update returning a only boolean, we need the config in the _read() function This was the result of a last minute refactor and inadequate testing. Added test coverage to prevent errors around this in the future --- palm/plugins/base_plugin_config.py | 21 +++++---- palm/plugins/core/commands/cmd_plugin.py | 2 +- tests/unit/test_plugin_config.py | 56 ++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 12 deletions(-) diff --git a/palm/plugins/base_plugin_config.py b/palm/plugins/base_plugin_config.py index 9f48028..563cac2 100644 --- a/palm/plugins/base_plugin_config.py +++ b/palm/plugins/base_plugin_config.py @@ -1,7 +1,8 @@ -from abc import ABC, abstractmethod import click -from pathlib import Path import yaml +from typing import Tuple +from abc import ABC, abstractmethod +from pathlib import Path from pydantic import BaseModel, ValidationError from palm.palm_exceptions import InvalidConfigError @@ -26,11 +27,12 @@ def set(self) -> dict: """ pass - def update(self) -> bool: + def update(self) -> Tuple[bool, dict]: """Updates the config file with configuration provided by the set method Returns: - bool: True if the config was updated, False if it was not + Tuple[bool, dict]: Returns a tuple of (True, config) if the config was + updated successfully, and (False, {}) if it was not. """ try: config = self.set() @@ -38,11 +40,11 @@ def update(self) -> bool: self._write(config) except InvalidConfigError as e: click.secho(str(e), fg="red") - return False + raise e except Exception as e: click.secho(str(e), fg="red") - return False - return True + return (False, {}) + return (True, config) def get(self) -> BaseModel: if not self.config_path.exists(): @@ -75,10 +77,7 @@ def _read(self) -> dict: plugin_config = palm_config.get('plugin_config', {}).get(self.plugin_name, {}) if not plugin_config: - plugin_config = self.update() - - if not self.validate(plugin_config): - raise InvalidConfigError + _, plugin_config = self.update() return plugin_config diff --git a/palm/plugins/core/commands/cmd_plugin.py b/palm/plugins/core/commands/cmd_plugin.py index 7a53f5c..9337175 100644 --- a/palm/plugins/core/commands/cmd_plugin.py +++ b/palm/plugins/core/commands/cmd_plugin.py @@ -134,7 +134,7 @@ def configure(environment, name: Optional[str]): click.secho(f"Plugin {name} not installed in this project", fg="red") return - success = plugin.config.update() + success, _ = plugin.config.update() if success: click.secho(f"Plugin {plugin.name} configured successfully", fg="green") else: diff --git a/tests/unit/test_plugin_config.py b/tests/unit/test_plugin_config.py index 058da55..f506361 100644 --- a/tests/unit/test_plugin_config.py +++ b/tests/unit/test_plugin_config.py @@ -1,5 +1,6 @@ import pytest import yaml +from unittest import mock from pathlib import Path from palm.palm_exceptions import InvalidConfigError @@ -42,3 +43,58 @@ def test_get_config(mock_plugin_config, tmp_path, monkeypatch): monkeypatch.setattr(config, "config_path", config_path) assert config.get() == {"value": "foo"} + +@pytest.mark.parametrize("config", [{"value": "foo"}]) +def test_update_config_returns_success_and_config(mock_plugin_config, tmp_path, monkeypatch): + config = mock_plugin_config + config_path = tmp_path / "config.yaml" + config_path.write_text(yaml.dump({'image_name': 'palm-test'})) + monkeypatch.setattr(config, "config_path", config_path) + + assert config.update() == (True, {"value": "foo"}) + +@pytest.mark.parametrize("config", [{"value": "foo"}]) +def test_update_config_writes_to_file(mock_plugin_config, tmp_path, monkeypatch): + config = mock_plugin_config + config_path = tmp_path / "config.yaml" + config_path.write_text(yaml.dump({'image_name': 'palm-test'})) + monkeypatch.setattr(config, "config_path", config_path) + + config.update() + + config = yaml.safe_load(config_path.read_text()) + assert config["plugin_config"]["test"]["value"] == "foo" + + +@pytest.mark.parametrize("config", [{}]) +def test_update_config_raises_for_invalid_config(mock_plugin_config, tmp_path, monkeypatch): + config = mock_plugin_config + config_path = tmp_path / "config.yaml" + config_path.write_text(yaml.dump({'image_name': 'palm-test'})) + monkeypatch.setattr(config, "config_path", config_path) + + with pytest.raises(InvalidConfigError): + config.update() + +@pytest.mark.parametrize("config", [{}]) +def test_read_config_with_update(mock_plugin_config, tmp_path, monkeypatch): + config = mock_plugin_config + config_path = tmp_path / "config.yaml" + config_path.write_text(yaml.dump({'image_name': 'palm-test'})) + monkeypatch.setattr(config, "config_path", config_path) + monkeypatch.setattr(config, "update", lambda: (True, {"value": "bar"})) + + assert config._read() == {"value": "bar"} + + +@pytest.mark.parametrize("config", [{}]) +def test_read_config_existing(mock_plugin_config, tmp_path, monkeypatch): + config = mock_plugin_config + config_path = tmp_path / "config.yaml" + config_path.write_text(yaml.dump({'plugin_config': {'test': {'value': 'foo'}}})) + monkeypatch.setattr(config, "config_path", config_path) + m = mock.Mock() + monkeypatch.setattr(config, "set", m) + + assert config._read() == {"value": "foo"} + assert m.called == False \ No newline at end of file From 46984fe168810cbc8e1888b93d928baaa94231bc Mon Sep 17 00:00:00 2001 From: Jake Beresford Date: Wed, 23 Nov 2022 11:59:37 -0500 Subject: [PATCH 2/2] Linting fixes --- tests/unit/test_plugin_config.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/tests/unit/test_plugin_config.py b/tests/unit/test_plugin_config.py index f506361..6c1b37a 100644 --- a/tests/unit/test_plugin_config.py +++ b/tests/unit/test_plugin_config.py @@ -44,8 +44,11 @@ def test_get_config(mock_plugin_config, tmp_path, monkeypatch): assert config.get() == {"value": "foo"} + @pytest.mark.parametrize("config", [{"value": "foo"}]) -def test_update_config_returns_success_and_config(mock_plugin_config, tmp_path, monkeypatch): +def test_update_config_returns_success_and_config( + mock_plugin_config, tmp_path, monkeypatch +): config = mock_plugin_config config_path = tmp_path / "config.yaml" config_path.write_text(yaml.dump({'image_name': 'palm-test'})) @@ -53,6 +56,7 @@ def test_update_config_returns_success_and_config(mock_plugin_config, tmp_path, assert config.update() == (True, {"value": "foo"}) + @pytest.mark.parametrize("config", [{"value": "foo"}]) def test_update_config_writes_to_file(mock_plugin_config, tmp_path, monkeypatch): config = mock_plugin_config @@ -67,7 +71,9 @@ def test_update_config_writes_to_file(mock_plugin_config, tmp_path, monkeypatch) @pytest.mark.parametrize("config", [{}]) -def test_update_config_raises_for_invalid_config(mock_plugin_config, tmp_path, monkeypatch): +def test_update_config_raises_for_invalid_config( + mock_plugin_config, tmp_path, monkeypatch +): config = mock_plugin_config config_path = tmp_path / "config.yaml" config_path.write_text(yaml.dump({'image_name': 'palm-test'})) @@ -76,6 +82,7 @@ def test_update_config_raises_for_invalid_config(mock_plugin_config, tmp_path, m with pytest.raises(InvalidConfigError): config.update() + @pytest.mark.parametrize("config", [{}]) def test_read_config_with_update(mock_plugin_config, tmp_path, monkeypatch): config = mock_plugin_config @@ -97,4 +104,4 @@ def test_read_config_existing(mock_plugin_config, tmp_path, monkeypatch): monkeypatch.setattr(config, "set", m) assert config._read() == {"value": "foo"} - assert m.called == False \ No newline at end of file + assert m.called == False