spec: remove backticks and use new logging #1954

kosstennbl · 2024-04-08T15:16:01Z

Replace all backtick command execution with ShellCmd module.
Use Log instead of LOGGING module.
Fix edge-case issues caused by logging refactor.
Remove commented-out code.

Commit looks scary, but most of the code is changed with a couple of regexes, only small amount of collisions that were needed to be resolved manually is present.

Issues:

REF: #1495

No changes to documentation is needed.
Was tested for successful compilation.
Needs testing with github actions (some of the tests are hard to test on local environments)

spec/5g/core_spec.cr

spec/cnf_testsuite_all/cnf_testsuite_microservice_spec.cr

spec/cnf_testsuite_all/cnf_testsuite_network_chaos_spec.cr

src/tasks/utils/utils.cr

HashNuke · 2024-04-09T07:17:55Z

ShellCmd.run is more suitable for commands that either output stdout on success or stderr on failure. The stderr is output in the end after the stdout is logged.

The cnf-testsuite commands could be outputting both due to the nature of the integrations.
In the logs when looking into issues, it would help to see the output in the same order as the command's output rather than all the stdouts grouped first, followed by all the stderr outputs.

It would be better to use the same variable for collecting both stderr and stdout. That is a case for for a new function separate from ShellCmd.run.

kosstennbl · 2024-04-09T08:07:06Z

ShellCmd.run is more suitable for commands that either output stdout on success or stderr on failure. The stderr is output in the end after the stdout is logged.

The cnf-testsuite commands could be outputting both due to the nature of the integrations.

In the logs when looking into issues, it would help to see the output in the same order as the command's output rather than all the stdouts grouped first, followed by all the stderr outputs.

It would be better to use the same variable for collecting both stderr and stdout. That is a case for for a new function separate from ShellCmd.run.

As we have discussed earlier - agree, that sounds like a nice addition, but I'm not sure that it should be implemented in the scope of this PR.
As these changes for logging and backticks are quite wide and can bring confusion in some places - I'd rather prefer fixing (if needed) and merging this one, and then - creating new PR with addition of second command and using it where needed.

martin-mat

lgtm

kosstennbl · 2024-04-17T08:35:27Z

@HashNuke @wavell @agentpoyo Please run tests

HashNuke · 2024-04-19T11:29:06Z

There are three tags that are failing in the build. I've run them a few times.
I've tried re-triggering a build here to confirm the failures - https://github.com/cnti-testcatalog/testsuite/actions/runs/8750289901

kosstennbl · 2024-04-19T12:20:59Z

@HashNuke
Thanks for testing, observability and platform:observability are fixed.
Memory hog failure is probably happening due to #1973

HashNuke

Posted about an error with screenshot.

HashNuke · 2024-04-21T05:36:37Z

spec/platform/observability_spec.cr

-      LOGGING.info resp
-      $?.success?.should be_true
+      Log.info { resp }
+      result[:status].success?.should be_true


Please look into this line. Below is the error from a build.

HashNuke · 2024-04-21T05:38:14Z

@kosstennbl platform:observability spec still throws an error. Please check screenshot in review.
Also, this PR has conflicts now.

kosstennbl · 2024-04-22T14:54:54Z

@HashNuke Fixed conflicts, fixed platform:observability (it fails in my environment, but after fix - it looks like an environment issue rather than code issue)

HashNuke · 2024-04-22T22:57:35Z

platform:observability is still failing in the latest build - https://github.com/cnti-testcatalog/testsuite/actions/runs/8791269449

Screenshot of the failure below.

kosstennbl · 2024-04-22T23:12:12Z

spec/platform/observability_spec.cr

 		  KubectlClient::Get.wait_for_install(deployment_name: "metrics-server")
-      response_s = `./cnf-testsuite platform:metrics_server poc`
-      LOGGING.info response_s
-      (/(PASSED){1}.*(Your platform is using the metrics server){1}/ =~ response_s).should_not be_nil
+      result = ShellCmd.run_testsuite("platform:metrics_server poc")
+      (/(PASSED){1}.*(Your platform is using the metrics server){1}/ =~ result[:output]).should_not be_nil
  ensure
      resp = Helm.uninstall("metrics-server")
-      LOGGING.info resp
+      Log.info { resp }
      $?.success?.should be_true
  end
 end


After some inspection of the changes for the failing test in this PR - it doesn't seem to me that this failure could be brought by them. Changes follow the same redesign rules as all other tests (which seem to be passing). Does this test succeed on other PRs?
@HashNuke

After further inspection - it seems that it was ensure block, which was causing issues. I'm not sure how and why it started failing after changing only logging for this block, but after last push it should be fine. Please rerun.

After further inspection - it seems that it was ensure block, which was causing issues. I'm not sure how and why it started failing after changing only logging for this block, but after last push it should be fine. Please rerun.
@HashNuke

REF: cnti-testcatalog#1495 Replace all backtick command execution with ShellCmd module. Use Log instead of LOGGING module. Fix edgecase issues caused by logging refactor. Remove commented-out code. Signed-off-by: Konstantin Yarovoy <[email protected]>

kosstennbl · 2024-04-29T14:48:07Z

Rebased after the "output refactor" merge, hopefully i haven't forgot to adapt anything.

HashNuke · 2024-05-07T14:57:48Z

Build passing - https://github.com/cnti-testcatalog/testsuite/actions/runs/8980950321.
Will merge.

kosstennbl requested review from denverwilliams, wvwatson and agentpoyo as code owners April 8, 2024 15:16

martin-mat requested changes Apr 8, 2024

View reviewed changes

HashNuke reviewed Apr 9, 2024

View reviewed changes

src/tasks/utils/utils.cr Outdated Show resolved Hide resolved

kosstennbl mentioned this pull request Apr 9, 2024

Spec tests shell command fix #1923

Closed

9 tasks

kosstennbl force-pushed the backtick_spec_fix branch 5 times, most recently from b2f088f to cdcbaf3 Compare April 11, 2024 08:20

kosstennbl changed the title ~~spec: remove backticks and use new logging~~ spec: refactor spec tests Apr 11, 2024

kosstennbl changed the title ~~spec: refactor spec tests~~ spec: remove backticks and use new logging Apr 12, 2024

kosstennbl force-pushed the backtick_spec_fix branch 2 times, most recently from 1ebb5ee to 2152086 Compare April 12, 2024 13:31

kosstennbl mentioned this pull request Apr 15, 2024

Enhancement: Add test name before and after the test in console output #1962

Merged

kosstennbl force-pushed the backtick_spec_fix branch 5 times, most recently from 244b178 to 5961641 Compare April 16, 2024 12:07

martin-mat self-requested a review April 16, 2024 12:58

martin-mat approved these changes Apr 16, 2024

View reviewed changes

kosstennbl force-pushed the backtick_spec_fix branch 2 times, most recently from 7c16f09 to ba63fc4 Compare April 19, 2024 12:19

HashNuke requested changes Apr 21, 2024

View reviewed changes

kosstennbl force-pushed the backtick_spec_fix branch 2 times, most recently from af7b839 to 98d16cb Compare April 22, 2024 14:46

kosstennbl commented Apr 22, 2024

View reviewed changes

kosstennbl force-pushed the backtick_spec_fix branch from 98d16cb to 744a33a Compare April 23, 2024 08:29

kosstennbl force-pushed the backtick_spec_fix branch from 744a33a to aa0da32 Compare April 29, 2024 14:46

kosstennbl mentioned this pull request May 3, 2024

[Feature] Uniform way of logging spec test output #1922

Closed

HashNuke merged commit 0893b2a into cnti-testcatalog:main May 7, 2024
103 of 178 checks passed

kosstennbl deleted the backtick_spec_fix branch May 27, 2024 08:34

kosstennbl mentioned this pull request Sep 4, 2024

[BUG] Remove Log.info wrappers when running shell commands in spec tests #1495

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

spec: remove backticks and use new logging #1954

spec: remove backticks and use new logging #1954

kosstennbl commented Apr 8, 2024 •

edited

Loading

HashNuke commented Apr 9, 2024

kosstennbl commented Apr 9, 2024

martin-mat left a comment

kosstennbl commented Apr 17, 2024

HashNuke commented Apr 19, 2024

kosstennbl commented Apr 19, 2024

HashNuke left a comment

HashNuke Apr 21, 2024

HashNuke commented Apr 21, 2024 •

edited

Loading

kosstennbl commented Apr 22, 2024

HashNuke commented Apr 22, 2024

kosstennbl Apr 22, 2024 •

edited

Loading

kosstennbl Apr 23, 2024

kosstennbl Apr 23, 2024 •

edited

Loading

kosstennbl commented Apr 29, 2024 •

edited

Loading

HashNuke commented May 7, 2024

spec: remove backticks and use new logging #1954

spec: remove backticks and use new logging #1954

Conversation

kosstennbl commented Apr 8, 2024 • edited Loading

Issues:

HashNuke commented Apr 9, 2024

kosstennbl commented Apr 9, 2024

martin-mat left a comment

Choose a reason for hiding this comment

kosstennbl commented Apr 17, 2024

HashNuke commented Apr 19, 2024

kosstennbl commented Apr 19, 2024

HashNuke left a comment

Choose a reason for hiding this comment

HashNuke Apr 21, 2024

Choose a reason for hiding this comment

HashNuke commented Apr 21, 2024 • edited Loading

kosstennbl commented Apr 22, 2024

HashNuke commented Apr 22, 2024

kosstennbl Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

kosstennbl Apr 23, 2024

Choose a reason for hiding this comment

kosstennbl Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

kosstennbl commented Apr 29, 2024 • edited Loading

HashNuke commented May 7, 2024

kosstennbl commented Apr 8, 2024 •

edited

Loading

HashNuke commented Apr 21, 2024 •

edited

Loading

kosstennbl Apr 22, 2024 •

edited

Loading

kosstennbl Apr 23, 2024 •

edited

Loading

kosstennbl commented Apr 29, 2024 •

edited

Loading