Skip to content

Rewrite taskflow-mapping argument validation#21759

Merged
uranusjr merged 1 commit intoapache:mainfrom
astronomer:taskflow-map-consider-ti-context
Feb 24, 2022
Merged

Rewrite taskflow-mapping argument validation#21759
uranusjr merged 1 commit intoapache:mainfrom
astronomer:taskflow-map-consider-ti-context

Conversation

@uranusjr
Copy link
Copy Markdown
Member

This new check now takes account of ti context variables and block
mapping/assigning to them. A few tests are added to validate the
behavior.


^ 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.

@github-actions
Copy link
Copy Markdown

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Feb 23, 2022
@uranusjr
Copy link
Copy Markdown
Member Author

Hmm, this is failing with a seemingly unrelated error:

_________________ TestTriggererCommand.test_capacity_argument __________________
  
  self = <tests.cli.commands.test_triggerer_command.TestTriggererCommand testMethod=test_capacity_argument>
  mock_scheduler_job = <MagicMock name='TriggererJob' id='140183951380688'>
  
      @pytest.mark.skipif(not PY37, reason="triggerer subcommand only works with Python 3.7+")
      @mock.patch("airflow.cli.commands.triggerer_command.TriggererJob")
      def test_capacity_argument(
          self,
          mock_scheduler_job,
      ):
          """Ensure that the capacity argument is passed correctly"""
          args = self.parser.parse_args(['triggerer', '--capacity=42'])
          triggerer_command.triggerer(args)
  >       mock_scheduler_job.assert_called_once_with(capacity="42")
  
  tests/cli/commands/test_triggerer_command.py:46: 
  _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
  /usr/local/lib/python3.7/unittest/mock.py:889: in assert_called_once_with
      return self.assert_called_with(*args, **kwargs)
  _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
  
  _mock_self = <MagicMock name='TriggererJob' id='140183951380688'>, args = ()
  kwargs = {'capacity': '42'}, expected = ((), {'capacity': '42'})
  _error_message = <function NonCallableMock.assert_called_with.<locals>._error_message at 0x7f7f1dceb050>
  actual = call(capacity=42), cause = None
  
      def assert_called_with(_mock_self, *args, **kwargs):
          """assert that the mock was called with the specified arguments.
      
          Raises an AssertionError if the args and keyword args passed in are
          different to the last call to the mock."""
          self = _mock_self
          if self.call_args is None:
              expected = self._format_mock_call_signature(args, kwargs)
              raise AssertionError('Expected call: %s\nNot called' % (expected,))
      
          def _error_message():
              msg = self._format_mock_failure_message(args, kwargs)
              return msg
          expected = self._call_matcher((args, kwargs))
          actual = self._call_matcher(self.call_args)
          if expected != actual:
              cause = expected if isinstance(expected, Exception) else None
  >           raise AssertionError(_error_message()) from cause
  E           AssertionError: Expected call: TriggererJob(capacity='42')
  E           Actual call: TriggererJob(capacity=42)
  
  /usr/local/lib/python3.7/unittest/mock.py:878: AssertionError

Investigating.

@uranusjr
Copy link
Copy Markdown
Member Author

uranusjr commented Feb 24, 2022

Looks like #21753 is the cause. Fixing that first. Actually, seems to pass on main? Rebasing.

This new check now takes account of ti context variables and block
mapping/assigning to them. A few tests are added to validate the
behavior.
@uranusjr uranusjr force-pushed the taskflow-map-consider-ti-context branch from b6d9a64 to 0ac59bb Compare February 24, 2022 07:40
@uranusjr
Copy link
Copy Markdown
Member Author

Provider-building job failed without logs. Hopefully unrelated; can’t see how it could be.

@uranusjr uranusjr merged commit 2da207d into apache:main Feb 24, 2022
@uranusjr uranusjr deleted the taskflow-map-consider-ti-context branch February 24, 2022 08:38
rustikk pushed a commit to rustikk/airflow that referenced this pull request Feb 25, 2022
@jedcunningham jedcunningham added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Feb 28, 2022
@jedcunningham jedcunningham added this to the Airflow 2.3.0 milestone Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dynamic-task-mapping AIP-42 changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) full tests needed We need to run full set of tests for this PR to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants