Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix help message display for dags test subcommand #8552

Merged
merged 1 commit into from
Apr 25, 2020

Conversation

houqp
Copy link
Member

@houqp houqp commented Apr 25, 2020

Without this fix, airflow dags test crashes with the following error:

    item_help = join([func(*args) for func, args in self.items])
  File "/usr/local/Cellar/python/3.7.5/Frameworks/Python.framework/Versions/3.7/lib/python3.7/argparse.py", line 531, in _format_action
    help_text = self._expand_help(action)
  File "/usr/local/Cellar/python/3.7.5/Frameworks/Python.framework/Versions/3.7/lib/python3.7/argparse.py", line 620, in _expand_help
    return self._get_help_string(action) % params
TypeError: unsupported operand type(s) for %: 'tuple' and 'dict'

Make sure to mark the boxes below before creating PR: [x]

  • Description above provides context of the change
  • Unit tests coverage for changes (not needed for documentation changes)
  • Target Github ISSUE in description if exists
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • I will engage committers as explained in Contribution Workflow Example.

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.
Read the Pull Request Guidelines for more information.

@mik-laj
Copy link
Member

mik-laj commented Apr 25, 2020

Can you add test to avoid regression? I think something similar to the one below is enough.

tests/cli/test_cli_parser.py
    def test_should_display_helps(self):
        parser = cli_parser.get_parser()

        all_command_as_args = sum([
            [[top_commaand.name]]
            if isinstance(top_commaand, cli_parser.ActionCommand)
            else [
                [top_commaand.name, nested_command.name] for nested_command in top_commaand.subcommands
            ]

            for top_commaand in cli_parser.airflow_commands
        ], [])
        for cmd_args in all_command_as_args:
            with self.assertRaises(SystemExit):
                parser.parse_args([*cmd_args, '--help'])

Copy link
Member

@zhongjiajie zhongjiajie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM, but still need add test as Kamil said

@zhongjiajie
Copy link
Member

BTW, Kamil test is awesome.

@turbaszek
Copy link
Member

turbaszek commented Apr 25, 2020

FYI flat map suggest by Kamil isn't best in terms of performance but it doesn't matter here probably.
https://dev.to/turbaszek/flat-map-in-python-3g98

@kaxil
Copy link
Member

kaxil commented Apr 25, 2020

FYI flat map suggest by Kamil isn't best in terms of performance but it doesn't matter here probably.
https://dev.to/turbaszek/flat-map-in-python-3g98

Nice blog post @turbaszek

@potiuk potiuk merged commit 5a47e80 into apache:master Apr 25, 2020
@potiuk
Copy link
Member

potiuk commented Apr 25, 2020

I merged it too quick - did not notice the last comment but indeed @hoqp -> tests would be awesome :)

houqp pushed a commit to houqp/airflow that referenced this pull request Apr 25, 2020
@houqp houqp deleted the dags_test_fix branch April 25, 2020 22:29
@houqp
Copy link
Member Author

houqp commented Apr 25, 2020

@mik-laj @zhongjiajie @potiuk please see PR #8561 for unit test.

Also thanks @turbaszek for the optimization tip. Like you said not going to matter in this case, but it's good to know. I changed mik-laj's implementation to use nested list comprehension based on the blog post you shared.

mik-laj pushed a commit that referenced this pull request Apr 26, 2020
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.

6 participants