cnti-testcatalog / testsuite

📞📱☎️📡🌐 Cloud Native Telecom Initiative (CNTI) Test Catalog is a tool to check for and provide feedback on the use of K8s + cloud native best practices in networking applications and platforms
https://wiki.lfnetworking.org/display/LN/Test+Catalog
Apache License 2.0
173 stars 71 forks source link

spec: remove backticks and use new logging #1954

Closed kosstennbl closed 4 months ago

kosstennbl commented 5 months ago

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)

HashNuke commented 5 months ago

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.

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 commented 5 months ago

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.

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.

kosstennbl commented 5 months ago

@HashNuke @wavell @agentpoyo Please run tests

HashNuke commented 5 months ago

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

CleanShot 2024-04-19 at 16 57 47@2x

kosstennbl commented 5 months ago

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

HashNuke commented 5 months ago

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

kosstennbl commented 5 months ago

@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 commented 5 months ago

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

Screenshot of the failure below.

CleanShot 2024-04-23 at 04 26 28@2x

kosstennbl commented 5 months ago

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

HashNuke commented 4 months ago

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