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

Spec tests shell command fix #1923

Closed
wants to merge 3 commits into from

Conversation

macaktom
Copy link
Contributor

@macaktom macaktom commented Mar 6, 2024

Description

This PR will remove shell commands from Log.info wrapper.

Part of PR is also removal of command ./cnf-testsuite setup, which is used two times in microservice_spec.cr before_all construct.

PR only provides quick fix of issue, where spec tests fail, if shell command is run in Log.info and log level error is used. To avoid repetition and add uniform way of logging spec tests I created different task for that (#1922), as that's not point of this fix.

Issues:

Refs: #1495

How has this been tested:

  1. Run spec tests in debug mode
  2. Run spec tests in error mode

Types of changes:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Checklist:

Documentation

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • No updates required.

Code Review

  • Does the test handle fatal exceptions, ie. rescue block

Issue

  • Tasks in issue are checked off

Copy link
Collaborator

@HashNuke HashNuke left a comment

Choose a reason for hiding this comment

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

When using backticks to run shell commands the output is printed to stdout.

So when log level is set to info, the output would be printed as log lines (to stdout), as well as to stdout again (for the backticks).

For the intended outcome of this PR, we could use the Process module.

There is also a ShellCmd module within the project that can be used/repurposed for the intended outcome of this PR. This wraps the Process module’s function call.

@HashNuke
Copy link
Collaborator

HashNuke commented Apr 2, 2024

I was wrong about the behaviour of backticks. I just tested it. But it still applies to stderr. If we use backticks, we will get some lines in stderr and some lines in stdout (in the log block) if the command outputs both.

This is the ShellCmd helper module that runs the command, and then outputs the command's output as a part of Log blocks -

module ShellCmd
def self.run(cmd, log_prefix, force_output=false)
Log.info { "#{log_prefix} command: #{cmd}" }
status = Process.run(
cmd,
shell: true,
output: output = IO::Memory.new,
error: stderr = IO::Memory.new
)
if force_output == false
Log.debug { "#{log_prefix} output: #{output.to_s}" }
else
Log.info { "#{log_prefix} output: #{output.to_s}" }
end
# Don't have to output log line if stderr is empty
if stderr.to_s.size > 1
Log.info { "#{log_prefix} stderr: #{stderr.to_s}" }
end
{status: status, output: output.to_s, error: stderr.to_s}
end
end

If there is missing functionality with the ShellCmd module, please feel free to update it as required.

CleanShot 2024-04-02 at 20 59 46@2x

@kosstennbl
Copy link
Collaborator

kosstennbl commented Apr 3, 2024

Should all backtick usage be refactored to ShellCmd module?
From my point of view - it makes sense, I'll start on it for all spec tests.
I'll create new PR and link it there, @macaktom wouldn't be continuing this one.

@kosstennbl
Copy link
Collaborator

kosstennbl commented Apr 9, 2024

New PR: #1954
This can be closed

@HashNuke HashNuke closed this Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants