Skip to content

Conversation

@khalidmammadov
Copy link
Contributor

@khalidmammadov khalidmammadov commented Nov 28, 2021

These test (first 2) cases were broken due to missing self.dagbag and dag_id=test_run_fork_has_external_executor_id.

This PR fixes the issues and makes the test inline with the suite logic/setup and preserves test case's objective.

Test cases fixed in this PR:

  • test_external_executor_id_present_for_fork_run_task
  • test_external_executor_id_present_for_process_run_task
  • test_logging_with_run_task
  • test_log_file_template_with_run_task

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.

@khalidmammadov khalidmammadov changed the title Fixing failing test test_external_executor_id_present_for_fork_run_task Fixing failing quarantined test cases in test_task_command Nov 28, 2021
@potiuk
Copy link
Member

potiuk commented Nov 28, 2021

cc: @uranusjr ?

@khalidmammadov
Copy link
Contributor Author

There is only one test case (test_logging_with_run_task_subprocess) left unfixed in this suite (tests.cli.commands.test_task_command.TestLogsfromTaskRunCommand) so we can un-quarantine it and that is dependent on #19845. Which will help me to fix that last remaining test case as well.
Additionally, some test cases in this suite also being fixed by #19858

@khalidmammadov
Copy link
Contributor Author

This one also related #19132 and from the same file but not one of the quarantined ones

@potiuk
Copy link
Member

potiuk commented Dec 4, 2021

can you please rebase it @khalidmammadov ?

@khalidmammadov khalidmammadov force-pushed the fix_test_external_executor_id_present_for_fork_run_task branch from 6d191ba to 9244f09 Compare December 5, 2021 11:45
@khalidmammadov
Copy link
Contributor Author

@potiuk done

@potiuk
Copy link
Member

potiuk commented Dec 5, 2021

Those look good. However - can we just remove the quarantined markers from the class and add them to the two test methods that are still failiing? this will make only the two failing tests skipped @khalidmammadov

@khalidmammadov
Copy link
Contributor Author

@potiuk done.
I have also got some draft fix for test_logging_with_run_task_subprocess. Will do PR when finished, busy with MyPy work

@potiuk potiuk merged commit 9319a31 into apache:main Dec 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants