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

Refs #7451 - allow skipping message tests #292

Merged
merged 1 commit into from
Nov 7, 2018

Conversation

tstrachota
Copy link
Member

No description provided.

Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

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

I think we could extend the checking process somehow, see the inline comment.

all_subcommands(main_cmd, parent).each do |cmd|
next if except.any? { |except_cmd| cmd == except_cmd }
Copy link
Member

@ofedoren ofedoren Oct 26, 2018

Choose a reason for hiding this comment

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

This will skip the whole command, but what if we want to skip checking the success message only and be sure that description hasn't a period at the end?

I'd suggest some changes:

  def check_command_messages(cmd, except = nil)
    except ||= []
    cmd.recognised_options.each do |opt|
      check_option_description(cmd, opt)
    end
    # we could skip checking only particular messages from a command
    refute_msg_period(cmd, :desc) unless except.include?(:desc)
    assert_msg_period(cmd, :success_message) unless except.include?(:success_message)
    refute_msg_period(cmd, :failure_message) unless except.include?(:failure_message)
  end

  def check_all_command_messages(main_cmd, parent = HammerCLI::AbstractCommand, opts = {})
    except = opts[:except] || {}

    all_subcommands(main_cmd, parent).each do |cmd|
      # skip the whole command if nothing particular was specified
      next if except.key?(cmd.to_s) && except[cmd.to_s].empty?
      it "test messages of #{cmd}" do
        check_command_messages(cmd, except[cmd.to_s])
      end
    end
  end

Those changes would lead to changes in theforeman/hammer-cli-foreman#352 as:

  check_all_command_messages(
    HammerCLI::MainCommand,
    HammerCLIForeman::Command,
    except: {
      'HammerCLIForeman::PersonalAccessToken::CreateCommand' => [:success_message]
    }
  )

@@ -42,8 +42,11 @@ def check_command_messages(cmd)
refute_msg_period(cmd, :failure_message)
end

def check_all_command_messages(main_cmd, parent=HammerCLI::AbstractCommand)
def check_all_command_messages(main_cmd, parent=HammerCLI::AbstractCommand, opts = {})
Copy link
Member

Choose a reason for hiding this comment

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

@tstrachota what do you think about keyword argument for except?

@tstrachota tstrachota changed the title Refx #7451 - allow skipping message tests Refs #7451 - allow skipping message tests Nov 1, 2018
@tstrachota
Copy link
Member Author

@mbacovsky @ofedoren updated

@ofedoren
Copy link
Member

ofedoren commented Nov 7, 2018

@tstrachota, works nice, I've got nothing to add :)

@mbacovsky
Copy link
Member

👍 Thanks @tstrachota, @ofedoren

@mbacovsky mbacovsky merged commit 61bd20f into theforeman:master Nov 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants