You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "slycyberguy (via GitHub)" <gi...@apache.org> on 2024/04/08 04:32:57 UTC
[PR] Add four unit tests for aws/utils [airflow]
slycyberguy opened a new pull request, #38820:
URL: https://github.com/apache/airflow/pull/38820
Unit tests for:
airflow/provider/amazon/aws/utils/sagemaker.py
airflow/provider/amazon/aws/utils/rds.py
airflow/provider/amazon/aws/utils/sqs.py
airflow/provider/amazon/aws/utils/tags.py
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
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](https://github.com/apache/airflow/tree/main/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add four unit tests for aws/utils [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #38820:
URL: https://github.com/apache/airflow/pull/38820#discussion_r1558099997
##########
tests/providers/amazon/aws/utils/test_sagemaker.py:
##########
Review Comment:
I didn't review the previous tests that have been removed from this PR, but the tests remaining seem to add coverage. Hard to tell for sure if they were generated (there are some weird names like `response_dict_dict` that seem unnatural).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add four unit tests for aws/utils [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on PR #38820:
URL: https://github.com/apache/airflow/pull/38820#issuecomment-2057236733
> > You will need to remove these lines from the project structure tests:
> > https://github.com/apache/airflow/blob/main/tests/always/test_project_structure.py#L85-L86
>
> Do you know if the others will be reviewing this PR any time soon? Also [PR 38819](https://github.com/apache/airflow/pull/38819) has been approved by one reviewer and since you're named as one of the reviewers, I was hoping you can review it when you get a chance. Thanks.
It's not really about the number of approvers, it was more that @potiuk and @hussein-awala had some dissenting opinions on the PR, so I wanted to give them the opportunity to review the current version after your changes and see if they find it more acceptable now. However, they do seem to be busy! So if no one checks in by end of today or tomorrow I can merge this. We can always make changes later :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add four unit tests for aws/utils [airflow]
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #38820:
URL: https://github.com/apache/airflow/pull/38820#discussion_r1555529961
##########
tests/providers/amazon/aws/utils/test_rds.py:
##########
Review Comment:
This test is useless
##########
tests/providers/amazon/aws/utils/test_sagemaker.py:
##########
Review Comment:
same here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add four unit tests for aws/utils [airflow]
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #38820:
URL: https://github.com/apache/airflow/pull/38820#issuecomment-2057425722
Awesome work, congrats on your first merged pull request! You are invited to check our [Issue Tracker](https://github.com/apache/airflow/issues) for additional contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add four unit tests for aws/utils [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #38820:
URL: https://github.com/apache/airflow/pull/38820#discussion_r1558170657
##########
tests/providers/amazon/aws/utils/test_sagemaker.py:
##########
Review Comment:
I think maybe `response_dict_json` would be more accurate since we're parsing it as json. Or maybe `response_nested_dict` if we really want to keep it dict.
But any who, naming aside. Thanks for adding the tests :slightly_smiling_face: and sorry you got some suspicious push back from us!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add four unit tests for aws/utils [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #38820:
URL: https://github.com/apache/airflow/pull/38820#discussion_r1555891937
##########
tests/providers/amazon/aws/utils/test_sagemaker.py:
##########
Review Comment:
I had the same impression that most of the tests produced / generated here are useless. I think it's not a good idea to clutter and make more of those tests that are really not testing anything - that's why in the other PR I've asked whether the tests were generated.
I think all those PRs where the tests were generated to start with and then replicated should not make it into our codebase.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add four unit tests for aws/utils [airflow]
Posted by "slycyberguy (via GitHub)" <gi...@apache.org>.
slycyberguy commented on code in PR #38820:
URL: https://github.com/apache/airflow/pull/38820#discussion_r1556846390
##########
tests/providers/amazon/aws/utils/test_sagemaker.py:
##########
Review Comment:
I removed those tests. I figured they were necessary because those services were listed in issue #35442
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add four unit tests for aws/utils [airflow]
Posted by "slycyberguy (via GitHub)" <gi...@apache.org>.
slycyberguy commented on PR #38820:
URL: https://github.com/apache/airflow/pull/38820#issuecomment-2048617564
> You will need to remove these lines from the project structure tests:
>
> https://github.com/apache/airflow/blob/main/tests/always/test_project_structure.py#L85-L86
I took care of it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add four unit tests for aws/utils [airflow]
Posted by "slycyberguy (via GitHub)" <gi...@apache.org>.
slycyberguy commented on code in PR #38820:
URL: https://github.com/apache/airflow/pull/38820#discussion_r1558165988
##########
tests/providers/amazon/aws/utils/test_sagemaker.py:
##########
Review Comment:
It's not generated. I tried to come up with a more descriptive name for the message samples. One was a list of dict and the other was a list of dict of dict. That's why I chose those names. Anyway, I just renamed the fixture labels to sample_message_1 and sample_message_2. Hope that works. Let me know your suggestions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add four unit tests for aws/utils [airflow]
Posted by "slycyberguy (via GitHub)" <gi...@apache.org>.
slycyberguy commented on PR #38820:
URL: https://github.com/apache/airflow/pull/38820#issuecomment-2054526341
> You will need to remove these lines from the project structure tests:
>
> https://github.com/apache/airflow/blob/main/tests/always/test_project_structure.py#L85-L86
Do you know if the others will be reviewing this PR any time soon? Also [PR 38819](https://github.com/apache/airflow/pull/38819) has been approved by one reviewer and since you're named as one of the reviewers, I was hoping you can review it when you get a chance. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add four unit tests for aws/utils [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #38820:
URL: https://github.com/apache/airflow/pull/38820#issuecomment-2057426525
Now those tests looked more useful
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add four unit tests for aws/utils [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #38820:
URL: https://github.com/apache/airflow/pull/38820
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add four unit tests for aws/utils [airflow]
Posted by "slycyberguy (via GitHub)" <gi...@apache.org>.
slycyberguy commented on code in PR #38820:
URL: https://github.com/apache/airflow/pull/38820#discussion_r1558189422
##########
tests/providers/amazon/aws/utils/test_sagemaker.py:
##########
Review Comment:
No problem. Thanks for the suggestions. I renamed them similar to your examples.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add four unit tests for aws/utils [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #38820:
URL: https://github.com/apache/airflow/pull/38820#discussion_r1558210182
##########
tests/providers/amazon/aws/utils/test_sagemaker.py:
##########
Review Comment:
Sounds good! I approved. We'll need have all conversations resolved before merging. I'll let @potiuk and @hussein-awala reply back to this thread to see if they have any concerns.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add four unit tests for aws/utils [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #38820:
URL: https://github.com/apache/airflow/pull/38820#discussion_r1555893343
##########
tests/providers/amazon/aws/utils/test_sagemaker.py:
##########
Review Comment:
Potential Value added vs. time needed to review/classify those tests do not seem to be worth it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org