You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org> on 2023/02/04 19:07:14 UTC
[GitHub] [airflow] Abhishek-kumar-samsung opened a new pull request, #29375: migrated some part of test always to pytest
Abhishek-kumar-samsung opened a new pull request, #29375:
URL: https://github.com/apache/airflow/pull/29375
Migrated some part of tests/always to pytest
related: https://github.com/apache/airflow/issues/29305
--
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
[GitHub] [airflow] Abhishek-kumar-samsung commented on a diff in pull request #29375: migrated some part of test always to pytest
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on code in PR #29375:
URL: https://github.com/apache/airflow/pull/29375#discussion_r1103849190
##########
tests/always/test_project_structure.py:
##########
@@ -90,8 +89,10 @@ def test_providers_modules_should_have_tests(self):
missing_tests_files = expected_test_files - expected_test_files.intersection(current_test_files)
- with self.subTest("Detect missing tests in providers module"):
+ try:
Review Comment:
@potiuk done as you have said.
--
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
[GitHub] [airflow] Abhishek-kumar-samsung commented on pull request #29375: migrated some part of test always to pytest
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #29375:
URL: https://github.com/apache/airflow/pull/29375#issuecomment-1427021438
@Taragolis migrated all tests/always files to pytest.
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #29375: migrated some part of test always to pytest
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29375:
URL: https://github.com/apache/airflow/pull/29375#discussion_r1103847025
##########
tests/always/test_project_structure.py:
##########
@@ -90,8 +89,10 @@ def test_providers_modules_should_have_tests(self):
missing_tests_files = expected_test_files - expected_test_files.intersection(current_test_files)
- with self.subTest("Detect missing tests in providers module"):
+ try:
Review Comment:
It's just unnecessary. What assert does it throws an exception, fail is just a shorthand for "assert false" so you are effectively running assert, catch exception, and run another assert. The way I proposed is the pytest-y way of doing what you want to do.
--
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
[GitHub] [airflow] Taragolis commented on pull request #29375: migrated some part of test always to pytest
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #29375:
URL: https://github.com/apache/airflow/pull/29375#issuecomment-1421009979
> In one file they are using self.subTest which was giving me problem while migrating,
As far as I remember:
1. In always subtest is useless and could be replaced by regular assert. And this test is broken and always success
2. In operators subtest also broken (do not cleanup stage between subtest run). In general all subtest could be rewrited by parametrised tests (`@pytest.mark.parametrize`)
> So i converted only a part of tests/always.
No problem, there is no target to do everything in one go
--
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
[GitHub] [airflow] Abhishek-kumar-samsung commented on pull request #29375: migrated some part of test always to pytest
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #29375:
URL: https://github.com/apache/airflow/pull/29375#issuecomment-1427106653
@potiuk i have added the changes as you have requested, and it is working fine, checks are also passed.
--
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
[GitHub] [airflow] Abhishek-kumar-samsung commented on pull request #29375: migrated some part of test always to pytest
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #29375:
URL: https://github.com/apache/airflow/pull/29375#issuecomment-1416828320
@Taragolis migrated some tests/always to pytest.
Please review.
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #29375: migrated some part of test always to pytest
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29375:
URL: https://github.com/apache/airflow/pull/29375#discussion_r1104263135
##########
tests/always/test_project_structure.py:
##########
@@ -90,8 +89,10 @@ def test_providers_modules_should_have_tests(self):
missing_tests_files = expected_test_files - expected_test_files.intersection(current_test_files)
- with self.subTest("Detect missing tests in providers module"):
+ try:
Review Comment:
Well.. :)
--
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
[GitHub] [airflow] Taragolis commented on a diff in pull request #29375: migrated some part of test always to pytest
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on code in PR #29375:
URL: https://github.com/apache/airflow/pull/29375#discussion_r1104156376
##########
tests/always/test_project_structure.py:
##########
@@ -90,8 +89,10 @@ def test_providers_modules_should_have_tests(self):
missing_tests_files = expected_test_files - expected_test_files.intersection(current_test_files)
- with self.subTest("Detect missing tests in providers module"):
+ try:
Review Comment:
Just for a record this test was broken before, and `missing_tests_files` always empty set.
--
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
[GitHub] [airflow] Abhishek-kumar-samsung commented on a diff in pull request #29375: migrated some part of test always to pytest
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on code in PR #29375:
URL: https://github.com/apache/airflow/pull/29375#discussion_r1103845238
##########
tests/always/test_project_structure.py:
##########
@@ -90,8 +89,10 @@ def test_providers_modules_should_have_tests(self):
missing_tests_files = expected_test_files - expected_test_files.intersection(current_test_files)
- with self.subTest("Detect missing tests in providers module"):
+ try:
Review Comment:
if you want, i can try your way @potiuk.
--
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
[GitHub] [airflow] Abhishek-kumar-samsung commented on a diff in pull request #29375: migrated some part of test always to pytest
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on code in PR #29375:
URL: https://github.com/apache/airflow/pull/29375#discussion_r1103847175
##########
tests/always/test_project_structure.py:
##########
@@ -90,8 +89,10 @@ def test_providers_modules_should_have_tests(self):
missing_tests_files = expected_test_files - expected_test_files.intersection(current_test_files)
- with self.subTest("Detect missing tests in providers module"):
+ try:
Review Comment:
oh okay, i will modify 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
[GitHub] [airflow] potiuk merged pull request #29375: migrated some part of test always to pytest
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #29375:
URL: https://github.com/apache/airflow/pull/29375
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #29375: migrated some part of test always to pytest
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29375:
URL: https://github.com/apache/airflow/pull/29375#discussion_r1103831108
##########
tests/always/test_project_structure.py:
##########
@@ -90,8 +89,10 @@ def test_providers_modules_should_have_tests(self):
missing_tests_files = expected_test_files - expected_test_files.intersection(current_test_files)
- with self.subTest("Detect missing tests in providers module"):
+ try:
Review Comment:
Hmm. That looks strange. I am not sure if I understand why we have try/Except at all (assert with description should be enough) and I am not sure what was the effect of self.subTest here. Can you please explain @Abhishek-kumar-samsung ?
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #29375: migrated some part of test always to pytest
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29375:
URL: https://github.com/apache/airflow/pull/29375#discussion_r1103847226
##########
tests/always/test_project_structure.py:
##########
@@ -90,8 +89,10 @@ def test_providers_modules_should_have_tests(self):
missing_tests_files = expected_test_files - expected_test_files.intersection(current_test_files)
- with self.subTest("Detect missing tests in providers module"):
+ try:
Review Comment:
And no. For me in this case try/except is actually confusing, because it makes the person reading it to think "is there any reason why this is done this way? Did I miss something?".
--
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
[GitHub] [airflow] Abhishek-kumar-samsung commented on a diff in pull request #29375: migrated some part of test always to pytest
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on code in PR #29375:
URL: https://github.com/apache/airflow/pull/29375#discussion_r1103844764
##########
tests/always/test_project_structure.py:
##########
@@ -90,8 +89,10 @@ def test_providers_modules_should_have_tests(self):
missing_tests_files = expected_test_files - expected_test_files.intersection(current_test_files)
- with self.subTest("Detect missing tests in providers module"):
+ try:
Review Comment:
I didn't knew if it can be done that way, i knew try except way, and tried testing by putting missing_tests_files as none, try except way was working fine so i kept that.
And also as per understanding, try except was much easy to understand.
--
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
[GitHub] [airflow] Abhishek-kumar-samsung commented on pull request #29375: migrated some part of test always to pytest
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #29375:
URL: https://github.com/apache/airflow/pull/29375#issuecomment-1427021612
@Taragolis @potiuk @eladkal all checks have passed.
Please review.
--
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
[GitHub] [airflow] Taragolis commented on pull request #29375: migrated some part of test always to pytest
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #29375:
URL: https://github.com/apache/airflow/pull/29375#issuecomment-1420932818
You could check previous PR with migration always to `pytest`. Potentially you could find useful information 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
[GitHub] [airflow] Abhishek-kumar-samsung commented on pull request #29375: migrated some part of test always to pytest
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #29375:
URL: https://github.com/apache/airflow/pull/29375#issuecomment-1420958891
@Taragolis ok i will try to see.
Actually tests/always has two files which need to be migrated to pytest.
In one file they are using self.subTest which was giving me problem while migrating, and other file in @parameterized.expand a function was passed which was giving some problem to me while migrating.
So i converted only a part of tests/always.
But i will try to cover other left part also.
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #29375: migrated some part of test always to pytest
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29375:
URL: https://github.com/apache/airflow/pull/29375#discussion_r1103844112
##########
tests/always/test_project_structure.py:
##########
@@ -90,8 +89,10 @@ def test_providers_modules_should_have_tests(self):
missing_tests_files = expected_test_files - expected_test_files.intersection(current_test_files)
- with self.subTest("Detect missing tests in providers module"):
+ try:
Review Comment:
I see. Makes sense for subtest removal.
But why not just this:
```
assert set() == missing_tests_files, "Detect missing tests in providers module"
```
##########
tests/always/test_project_structure.py:
##########
@@ -90,8 +89,10 @@ def test_providers_modules_should_have_tests(self):
missing_tests_files = expected_test_files - expected_test_files.intersection(current_test_files)
- with self.subTest("Detect missing tests in providers module"):
+ try:
Review Comment:
I see. Makes sense for subtest removal.
But why not just this:
```python
assert set() == missing_tests_files, "Detect missing tests in providers module"
```
--
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
[GitHub] [airflow] Abhishek-kumar-samsung commented on a diff in pull request #29375: migrated some part of test always to pytest
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on code in PR #29375:
URL: https://github.com/apache/airflow/pull/29375#discussion_r1103844764
##########
tests/always/test_project_structure.py:
##########
@@ -90,8 +89,10 @@ def test_providers_modules_should_have_tests(self):
missing_tests_files = expected_test_files - expected_test_files.intersection(current_test_files)
- with self.subTest("Detect missing tests in providers module"):
+ try:
Review Comment:
I didn't knew if it can be done that way, i knew try catch way, and tried testing by putting missing_tests_files as none, try except way was working fine so i kept that.
And also as per understanding, try except was much easy to understand.
--
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
[GitHub] [airflow] Abhiaps commented on pull request #29375: migrated some part of test always to pytest
Posted by "Abhiaps (via GitHub)" <gi...@apache.org>.
Abhiaps commented on PR #29375:
URL: https://github.com/apache/airflow/pull/29375#issuecomment-1421018357
> > In one file they are using self.subTest which was giving me problem while migrating,
>
> As far as I remember:
>
> 1. In always subtest is useless and could be replaced by regular assert. And this test is broken and always success
> 2. In operators subtest also broken (do not cleanup stage between subtest run). In general all subtest could be rewrited by parametrised tests (`@pytest.mark.parametrize`)
>
> > So i converted only a part of tests/always.
>
> No problem, there is no target to do everything in one go
😊Okay i will try
--
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
[GitHub] [airflow] Taragolis commented on pull request #29375: migrated some part of test always to pytest
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #29375:
URL: https://github.com/apache/airflow/pull/29375#issuecomment-1421011911
However still better to fix at list one module 😉
--
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
[GitHub] [airflow] Abhiaps commented on pull request #29375: migrated some part of test always to pytest
Posted by "Abhiaps (via GitHub)" <gi...@apache.org>.
Abhiaps commented on PR #29375:
URL: https://github.com/apache/airflow/pull/29375#issuecomment-1427076083
@potiuk Actually as per what I read subTest can be used if we need to do multiple tests with varying arguments. But here it was just a single test which can be written via simple assert only.
So I am not sure why they used subTest here.
In one more PR in which I migrated tests/operator test functions, there were several uses of subTest which makes a lot of sense, there they used for loop to create several subTests with varying arguments.
But here I don't think it makes much sense of writing subTest if we can simply write assert.
Just above in this PR only @Taragolis also commented that here subTest is useless and can be easily replaced by regular assert.
--
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
[GitHub] [airflow] Abhishek-kumar-samsung commented on a diff in pull request #29375: migrated some part of test always to pytest
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on code in PR #29375:
URL: https://github.com/apache/airflow/pull/29375#discussion_r1103835921
##########
tests/always/test_project_structure.py:
##########
@@ -90,8 +89,10 @@ def test_providers_modules_should_have_tests(self):
missing_tests_files = expected_test_files - expected_test_files.intersection(current_test_files)
- with self.subTest("Detect missing tests in providers module"):
+ try:
Review Comment:
I explained just below.
--
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