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

Use base aws classes in Amazon ECS Operators/Sensors/Triggers #36393

Merged
merged 5 commits into from
Dec 26, 2023

Conversation

jayceslesar
Copy link
Contributor

Similar to #35133 (relates to #35278)

Unsure how to get tests passing for test_hook_and_client in tests/providers/amazon/aws/operators/test_ecs.py, because it seems like it should just work 🤷

Copy link
Contributor

@Taragolis Taragolis left a comment

Choose a reason for hiding this comment

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

Unsure how to get tests passing for test_hook_and_client in tests/providers/amazon/aws/operators/test_ecs.py, because it seems like it should just work 🤷

The better is rewrite, because in this case it mock different object. You need to mock airflow.providers.amazon.aws.operators.ecs.EcsBaseOperator.aws_hook_class however it would not work because it would fail on subclass check

You could split it by two test in first just check that expected argument are passed into the hook constructor, something similar to the

def test_base_aws_op_attributes(self):
op = AthenaOperator(**self.default_op_kwargs)
assert op.hook.aws_conn_id == "aws_default"
assert op.hook._region_name is None
assert op.hook._verify is None
assert op.hook._config is None
assert op.hook.log_query is True
op = AthenaOperator(
**self.default_op_kwargs,
aws_conn_id="aws-test-custom-conn",
region_name="eu-west-1",
verify=False,
botocore_config={"read_timeout": 42},
log_query=False,
)
assert op.hook.aws_conn_id == "aws-test-custom-conn"
assert op.hook._region_name == "eu-west-1"
assert op.hook._verify is False
assert op.hook._config is not None
assert op.hook._config.read_timeout == 42
assert op.hook.log_query is False

And second test just mock hook property and check and check client property

airflow/providers/amazon/aws/operators/ecs.py Outdated Show resolved Hide resolved
airflow/providers/amazon/aws/sensors/ecs.py Outdated Show resolved Hide resolved
airflow/providers/amazon/aws/operators/ecs.py Outdated Show resolved Hide resolved
airflow/providers/amazon/aws/sensors/ecs.py Outdated Show resolved Hide resolved
@jayceslesar
Copy link
Contributor Author

@Taragolis thank you for the review -- all but mocking the hook property and client property should be all set. Pretty unsure how to best do that? Should I just test that the hook was not called and keep the client portion of the test the same?

@Taragolis
Copy link
Contributor

It could be something like that.

        with mock.patch.object(EcsBaseOperator, "hook", new_callable=mock.PropertyMock) as m:
            mocked_hook = mock.MagicMock(name="FakeHook")
            mocked_client = mock.MagicMock(name="FakeBoto3Client")
            mocked_hook.conn = mocked_client
            m.return_value = mocked_hook

            assert op.client == mocked_client
            m.assert_called_once()

@Taragolis
Copy link
Contributor

And after that you could remove test_hook_and_client because it not required anymore after that

@jayceslesar
Copy link
Contributor Author

Thank you for the example!

@Taragolis
Copy link
Contributor

Need to fix Static Checks:

  • you could run locally breeze static-checks {id-of-failed-check} --all-files e.g. breeze static-checks ruff --all-files
  • Another option run it thought pre-commit pre-commit run {id-of-failed-check} --all-files e.g. pre-commit run ruff --all-files

You could have a look what failed in Tests / Static checks (pull_request) logs

@potiuk potiuk merged commit 73d8794 into apache:main Dec 26, 2023
52 checks passed
Copy link

boring-cyborg bot commented Dec 26, 2023

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

abhishekbhakat pushed a commit to abhishekbhakat/my_airflow that referenced this pull request Mar 5, 2024
…#36393)

* Use base aws classes in Amazon ECS Operators/Sensors/Triggers

* remove redundant init and wrapper for the hook that essentially did nothing

* split out half of test

* change test to properly mock hook

* format
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants