add new clear_logfile method: by hholoubk · Pull Request #4095 · avocado-framework/avocado-vt

hholoubk · 2025-03-27T14:37:33Z

this will clear defined log_file - so the test will consider only the messages added in the test run.
previous log can be backuped to the tmp dir if requrired (the restore not yet prepared)

add new backup_logfile method: used in clear_logfile, but it can be used just as is

minor refactor to process not only bollean str_in_log variable (as it comes from test in my case as str)

dzhengfy · 2025-04-12T03:15:40Z

@hholoubk please make sure the CI static check is passed.

dzhengfy · 2025-04-12T03:31:00Z

    else:
        cmdRes = remote_old.run_remote_cmd(cmd, cmd_parms, runner_on_target)
+
+    if isinstance(str_in_log, str):


I do think we need to add this support. This function is an utility and docstring already has clear description that str_in_log variable is a boolean. So the user needs to give correct type for this parameter by themselves.

(did you mean ...that we DON"T need this support ... .. according rest of the comment it looks like this )
issue is .. that if it is named "str_in_log" .. than it is misleading name and the we should update the docstring ...
this way the utility will be more robust ... (as I spent one hour debugging why it doesn't work with "False" ...
and if the content of str_in_log is passed from parameter .. it was interpretted as string ...

what do you think?
I can do some performance testing how this will impact the test runs

sorry, I missed "not".
As you can see

def check_logfile( search_str, log_file, str_in_log=True, ===> this indicates the boolean, I think.

And there is also indicator in below codes:
if str_in_log == bool(int(cmdRes.exit_status)):

But anyway, an easy way to make better readability is we add type for parameter in docstring.
So could you help update the docstring?

For example:

:param str_in_log: bool, True if the file should include the given string, otherwise, False

dzhengfy · 2025-04-12T03:41:01Z

+    return backup_file
+
+
+def clear_logfile(log_file_name,


I suggest we make the function simple that it only does one thing to clear a specified log file. We do not need to embed the backup operation. Leave the backup operation to the end users if they need.

Yes ... make sense ... the function is now in utils_logfile.py ... and doesn't contain backup of file (I've fully removed the function, as it is not necessary for this case ... and ti will need agreement with team to properly design it as part of the utils_logfile.py

dzhengfy · 2025-04-12T03:41:54Z

+    :param backup_old_log: if True, the actual content of log_file will be backuped; default False
+
+    Returns:
+        str or None: The name of the backup file if created, otherwise None.


We just return True for clearing the log file successfully or False for failing to make it empty.

dzhengfy · 2025-04-12T03:48:28Z

    return obj_conf


+def backup_logfile(log_file, backup_file_name, backup_dir="/tmp"):


Suggest backup_dir=None

removed whole function - not needed for the test

dzhengfy · 2025-04-12T03:48:57Z

+    Returns:
+        str: The name of created backup file
+    """
+


We'd better provide an opportunity for the end users to define the final backup_file_name without changes (like + timestamp). So I would suggest below

if not backup_file_name: timestamp = time.strftime('%Y%m%d_%H%M%S') backup_file_name = f"{backup_file_name}_{timestamp}.log"

removed whole function - not needed for the test

dzhengfy · 2025-04-12T03:55:15Z

+
+    timestamp = time.strftime('%Y%m%d_%H%M%S')
+    backup_file_name = f"{backup_file_name}_{timestamp}.log"
+    backup_file = os.path.join(backup_dir, backup_file_name)


For backup_dir, its default is /tmp. Then the end user needs to remove the backup file by themselves.
I would suggest we use data_dir.get_tmp_dir() if the end user does not provide any value because the avocado-vt will help clear the files in data_dir.get_tmp_dir() at the end of the test for us.

So is it better if we use below:

if not backup_dir: backup_file = os.path.join(data_dir.get_tmp_dir(), backup_file_name)

removed whole function - not needed for the test

dzhengfy · 2025-04-12T04:07:27Z

    return obj_conf


+def backup_logfile(log_file, backup_file_name, backup_dir="/tmp"):


We do suggest not add any new function to libvirt.py any more because it is too long and not categorized.
Could you please move it to utils_logfile.py？

.. moved (just the clear_log_file ... not the backup, as it is not necessary ... and it would need properly design more things to reflect file structure

hholoubk · 2025-04-14T07:53:47Z

@dzhengfy statick check should be ok

dzhengfy

LGTM

Yingshun

Please fix the Static checks failures, thx!

Yingshun · 2025-04-25T02:58:07Z

+
+def clear_log_file(log_file_name, log_dir=None):
+    """
+    Clears the (e.g. VM) log file before a test run. closes the


let's complete the sentence, thx!

Yingshun · 2025-05-09T02:58:49Z

Still got errors in checks, plz fix them, thanks!

hholoubk · 2025-05-09T14:03:04Z

Still got errors in checks, plz fix them, thanks!

The previous errors were caused by some check limit ... it looks good now

Yingshun · 2025-05-12T01:14:04Z

@hholoubk Please also fix the problem in Static checks.

hholoubk · 2025-05-14T11:34:59Z

I hope I have finaly realized .. what should be in commit message.

- this will clear defined log_file - so the test will consider only the messages added in the test run. - previous log can be backuped to the tmp dir if requrired (the restore not yet prepared) add new backup_logfile method: used in clear_logfile, but it can be used just as is + minor refactor to process not only bollean str_in_log variable (as it comes from test in my case as str) Signed-off-by: hholoubk <hholoubk@redhat.com>

hholoubk mentioned this pull request Mar 27, 2025

automate VIRT-303225 - [vIOMMU] virsh reset when booting guest for 10+ times autotest/tp-libvirt#6249

Merged

dzhengfy requested review from Yingshun and dzhengfy April 9, 2025 09:55

dzhengfy reviewed Apr 12, 2025

View reviewed changes

hholoubk force-pushed the XXX-303225-add-clear-log-func branch from 7c565c9 to 998f80a Compare April 14, 2025 07:08

hholoubk force-pushed the XXX-303225-add-clear-log-func branch 3 times, most recently from 75fb96f to 71c0a88 Compare April 16, 2025 13:56

dzhengfy approved these changes Apr 21, 2025

View reviewed changes

Yingshun closed this Apr 25, 2025

Yingshun reopened this Apr 25, 2025

Yingshun reviewed Apr 25, 2025

View reviewed changes

dzhengfy requested a review from Yingshun May 9, 2025 02:53

Yingshun closed this May 9, 2025

Yingshun reopened this May 9, 2025

dzhengfy closed this May 9, 2025

dzhengfy reopened this May 9, 2025

hholoubk force-pushed the XXX-303225-add-clear-log-func branch from 4a20953 to 5cf3ca3 Compare May 9, 2025 10:03

hholoubk force-pushed the XXX-303225-add-clear-log-func branch 2 times, most recently from fea4d4f to 95760e2 Compare May 14, 2025 11:31

hholoubk force-pushed the XXX-303225-add-clear-log-func branch from 95760e2 to df0ba53 Compare May 14, 2025 11:42

Yingshun approved these changes May 14, 2025

View reviewed changes

Yingshun merged commit 58b79fa into avocado-framework:master May 14, 2025
50 checks passed

		return obj_conf


		def backup_logfile(log_file, backup_file_name, backup_dir="/tmp"):

Conversation

hholoubk commented Mar 27, 2025

Uh oh!

dzhengfy commented Apr 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dzhengfy Apr 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hholoubk Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hholoubk commented Apr 14, 2025

Uh oh!

dzhengfy left a comment

Choose a reason for hiding this comment

Uh oh!

Yingshun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Yingshun commented May 9, 2025

Uh oh!

hholoubk commented May 9, 2025

Uh oh!

Yingshun commented May 12, 2025

Uh oh!

hholoubk commented May 14, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dzhengfy Apr 12, 2025 •

edited

Loading

hholoubk Apr 16, 2025 •

edited

Loading