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 AWS Batch Operators/Sensors/Triggers #35226

Closed
wants to merge 1 commit into from

Conversation

Taragolis
Copy link
Contributor


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@Taragolis Taragolis force-pushed the amazon-batch-use-base branch from 317401d to 9b55492 Compare October 27, 2023 21:45
@@ -305,40 +337,55 @@ def test_monitor_job_with_logs(


class TestBatchCreateComputeEnvironmentOperator:
@pytest.fixture(autouse=True)
def setup_test_cases(self):
Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/apache/airflow/blob/main/TESTING.rst#id5

We are in the process of converting all unit tests to standard "asserts" and pytest fixtures so if you find some tests that are still using classic setUp/tearDown approach or unittest asserts, feel free to convert them to pytest.

Not sure whether we should usefixture in this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pytest.fixture(autouse=True)

It is pytest's builtin replacement for xunit-style setup / teardown with additional beneficials, like use another fixtures without some hacks

Use usefixture not always (almost never) detected by IDE's as autosuggestion it really annoying especially if it intends access to class instance attributes. It good ability if you need to assign clear fixture to specific namespaces (classes) in module and can't use:

  • @pytest.fixture(autouse=True, scope="class")
  • @pytest.fixture(autouse=True, scope="function")

Copy link
Member

Choose a reason for hiding this comment

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

It is pytest's builtin replacement for xunit-style setup / teardown with additional beneficials, like use another fixtures without some hacks

Yep, this is why I'm not sure whether this is something we want according to the document. I'm actually good with such things, but just want to make sure. If this is what we want, should we update the doc to reduce confusion?

Comment on lines +98 to +101
self.default_op_kwargs = dict(
task_id="test_batch_sensor",
job_id=JOB_ID,
)
Copy link
Member

Choose a reason for hiding this comment

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

According to https://github.com/apache/airflow/pull/33761/files, we probably would like to change it to

Suggested change
self.default_op_kwargs = dict(
task_id="test_batch_sensor",
job_id=JOB_ID,
)
self.default_op_kwargs = {
"task_id": "test_batch_sensor",
"job_id": JOB_ID,
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea why use literals always a better that call constructor, that is like personal preference over the strict rule. The only one exception which I know is: Empty dict literal {} should use over dict() and this one detected by black or/and ruff.

In particular this dictionary use for provide attributes to class constructor. So personally I found that in that case use dict constructor allow contributors to easy copy/paste from Operators/Sensors constructor attributes to default test arguments and vice versa without adding remove " to attribute names and replace : -> = -> :

Copy link
Contributor

Choose a reason for hiding this comment

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

The literal is slightly faster I believe (not the most compelling reason in this case, but worth noting since we're talking about it) but the more important thing is the keys are quoted in the literal. That way you don't have to worry about keywords or special characters in the key name. Which isn't a worry in this case, but I think it's best to choose one and stay consistent across the code base. So I'd also vote to move this to the literal {...}


@pytest.fixture(autouse=True)
def setup_test_cases(self):
self.default_op_kwargs = dict(
Copy link
Member

Choose a reason for hiding this comment

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

same as the suggestion above

@Taragolis
Copy link
Contributor Author

Yet another tab vs spaces and ' vs "

@Taragolis Taragolis closed this Oct 31, 2023
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