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

test: helper: Print stdout on timeout #163

Merged
merged 1 commit into from
Sep 29, 2023

Conversation

UVV-gh
Copy link
Contributor

@UVV-gh UVV-gh commented Sep 3, 2023

In case of timeout stdout in github actions is truncated, which makes it more difficult to understand the failure. This patch addresses this issue

for line in e.output.splitlines():
if line:
logger.info('stdout: %s', line)
raise
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't stderr then be printed as well?

Might it be an option to assign stoud and stderr to variables and handle printout in a finally clause?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, printing stderr makes sense. I'm not sure I'm getting the point with finally block. You mean to handle printout in finally instead of except block?

Copy link
Member

Choose a reason for hiding this comment

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

I ment combining the output for the good case and the error case, yes.

Something like

diff --git a/test/helper.py b/test/helper.py
index b067d6e..232aa5c 100644
--- a/test/helper.py
+++ b/test/helper.py
@@ -66,15 +66,21 @@ def run(command, *, timeout=30):
     logger = logger_from_command(command)
     logger.info('running: %s', command)
 
-    proc = subprocess.run(shlex.split(command), capture_output=True, text=True, check=False,
-                          timeout=timeout)
-
-    for line in proc.stdout.splitlines():
-        if line:
-            logger.info('stdout: %s', line)
-    for line in proc.stderr.splitlines():
-        if line:
-            logger.warning('stderr: %s', line)
+    try:
+        proc = subprocess.run(shlex.split(command), capture_output=True, text=True, check=False,
+                              timeout=timeout)
+        stdout = proc.stdout
+        stderr = proc.stderr
+    except subprocess.TimeoutExpired as e:
+        stdout = e.stdout
+        stderr = e.stderr
+    finally:
+        for line in stdout.splitlines():
+            if line:
+                logger.info('stdout: %s', line)
+        for line in stderr.splitlines():
+            if line:
+                logger.warning('stderr: %s', line)
 
     logger.info('exitcode: %d', proc.returncode)

But I have no clue how this actually differs from just printing the error itself and this solution also wouldn't allow to easily re-raise the error.

@Bastian-Krause do you have any better idea maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Published slightly modified version of your proposal

Signed-off-by: Vyacheslav Yurkov <[email protected]>
@UVV-gh UVV-gh force-pushed the feature/improve-helper branch from 31bfa4b to 5d0ea94 Compare September 7, 2023 17:35
Copy link
Member

@ejoerns ejoerns left a comment

Choose a reason for hiding this comment

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

Have no idea whether its good python practice or not to define methods in methods, but the approach looks otherwise fine to me 👍

@UVV-gh
Copy link
Contributor Author

UVV-gh commented Sep 26, 2023

Is there anything else left to fix before merge it?

@ejoerns ejoerns merged commit c57a1ee into rauc:master Sep 29, 2023
7 checks passed
@UVV-gh UVV-gh deleted the feature/improve-helper branch December 11, 2023 18:25
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.

2 participants