refactor: DBPT-1519 Refactor codebase command and tests #618
Conversation
…te from command test
❌ 1 Tests Failed:
View the top 1 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
chiaramapellimt
changed the title
chiaramapellimt
changed the title
| repository=repository, builder_version=builder_version | ||
| ) | ||
|
|
||
| self.echo_fn( |
There was a problem hiding this comment.
Thinking about if we ever had click (echo) functionality at command level, would these domain class commands require split up between echo's to support "echoing" progress
There was a problem hiding this comment.
sorry i dont get this can you explain this a bit?
There was a problem hiding this comment.
sorry, we can ignore my comment.
What I was thinking at the time was that the click functionality should be at the command layer but as you call it here would you need to split the function up if you were to move the echo to the command layer.
Although this does not matter as techinically self.echo_fn could be any logging function it could be strout but also anything else too.
WillGibson
changed the title
WillGibson
changed the title
| @@ -420,3 +420,78 @@ def get_vpc_info_by_name(session: Session, app: str, env: str, vpc_name: str) -> | |||
| raise AWSException(f"No matching security groups found in vpc '{vpc_name}'") | |||
|
|
|||
| return Vpc(subnets, sec_groups) | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
sorry to be a pain but do you thinking adding theese functions to a new providers/aws would split this logic from the old utilis?
| } | ||
|
|
||
|
|
||
| @patch("requests.get") |
There was a problem hiding this comment.
maybe overkill but is it possible to also inject requests like subprocess meaning you can mock this instead of patching?
There was a problem hiding this comment.
we only use it one place at the moment and it's a standard library so probably not great to add yet another 'injectable' argument to the class
| ) | ||
| @patch("dbt_platform_helper.commands.codebase.Codebase") | ||
| def test_codebase_prepare_calls_codebase_prepare_method(self, mock_codebase_object): | ||
| mock_codebase_object_instance = mock_codebase_object.return_value | ||
|
|
||
| result = CliRunner().invoke(prepare) |
There was a problem hiding this comment.
I will no longer be confused by this once the codebase command method has been renamed.
| result = CliRunner().invoke(prepare) | |
| result = CliRunner().invoke(codebase) |
It looked to me like we were asserting that the method we just invoked has been called.
| ) | ||
| assert result.exit_code == 1 |
There was a problem hiding this comment.
Isn't this behaviour the responsibility of domain.Codebase now?
So all we would want to be testing here is that domain.Codebase.build was called with the correct arguments?
Addresses DBTP-1519
Please add any relevant context for you pull request here, or delete this if none needed.
Checklist:
Title:
Description:
Tasks: