Use base aws classes in AWS Batch Operators/Sensors/Triggers#35226
Use base aws classes in AWS Batch Operators/Sensors/Triggers#35226Taragolis wants to merge 1 commit intoapache:mainfrom
Conversation
boring-cyborg
bot
added
area:providers
kind:documentation
provider:amazon
Taragolis
force-pushed
the
amazon-batch-use-base
branch
from
317401d to
9b55492
Compare
|
|
||
| class TestBatchCreateComputeEnvironmentOperator: | ||
| @pytest.fixture(autouse=True) | ||
| def setup_test_cases(self): |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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")
There was a problem hiding this comment.
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?
| self.default_op_kwargs = dict( | ||
| task_id="test_batch_sensor", | ||
| job_id=JOB_ID, | ||
| ) |
There was a problem hiding this comment.
According to https://github.com/apache/airflow/pull/33761/files, we probably would like to change it to
| 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, | |
| } |
There was a problem hiding this comment.
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 : -> = -> :
There was a problem hiding this comment.
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( |
|
Yet another tab vs spaces and ' vs " |
3 participants
^ 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.rstor{issue_number}.significant.rst, in newsfragments.