fix a issue as file or dirortry which is part of data_dir will retrun absolute file path

[PraveenPenguin]

As in misc test repo apply patch is failing which part of data dir as utility return relative path this patch address as it return absolute file path (file or dir)

[PraveenPenguin]

Sample run from misc test :
before fix:

avocado run eatmemory.py
Fetching asset from eatmemory.py:EatMemory.test
JOB ID     : 30f28ac7104b4f3ad3828ea73a43fa390c6985b1
JOB LOG    : /mnt/rootlv/avocado-fvt-wrapper/results/job-2024-05-14T03.37-30f28ac/job.log
 (1/1) eatmemory.py:EatMemory.test: STARTED
 (1/1) eatmemory.py:EatMemory.test: ERROR: Command 'patch -p1 < eatmemory.py.data/eatmem_getch.patch' failed.\nstdout: b''\nstderr: b'/bin/sh: eatmemory.py.data/eatmem_getch.patch: No such file or directory\n'\nadditional_info: None (0.09 s)
RESULTS    : PASS 0 | ERROR 1 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
JOB HTML   : /mnt/rootlv/avocado-fvt-wrapper/results/job-2024-05-14T03.37-30f28ac/results.html
JOB TIME   : 32.03 s

after fix

avocado run eatmemory.py
Fetching asset from eatmemory.py:EatMemory.test
JOB ID     : bbbea168b9ecfacd3f834ffa75ea24826729bacf
JOB LOG    : /mnt/rootlv/avocado-fvt-wrapper/results/job-2024-05-14T03.24-bbbea16/job.log
 (1/1) eatmemory.py:EatMemory.test: STARTED
 (1/1) eatmemory.py:EatMemory.test: PASS (7.74 s)
RESULTS    : PASS 1 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
JOB HTML   : /mnt/rootlv/avocado-fvt-wrapper/results/job-2024-05-14T03.24-bbbea16/results.html
JOB TIME   : 41.08 s

[PraveenPenguin]

@richtja can you please review this PR

[richtja]

Hi @PraveenPenguin even tho I understand that this will solve issue on misc test side I am not sure If we can make this change. The get_data is part of the avocado-instrumented core API and this change will affect all of our users, therefore we try to avoid such changes.

Is there a possibility to solve this on misc test side? And if it is not, I would propose to change the API in a less influencing way like this: get_data(self, filename, source=None, must_exist=True, abs_path=False) @clebergnu what do you think?

[PraveenPenguin]

Hi @PraveenPenguin even tho I understand that this will solve issue on misc test side I am not sure If we can make this change. The get_data is part of the avocado-instrumented core API and this change will affect all of our users, therefore we try to avoid such changes.

Is there a possibility to solve this on misc test side? And if it is not, I would propose to change the API in a less influencing way like this: get_data(self, filename, source=None, must_exist=True, abs_path=False) @clebergnu what do you think?

@richtja thanks for input ,yes make more sense to add abs_path as argument and misc test side we can overwrite this having asigining to True

[PraveenPenguin]

@richtja pushed changes , but CI is failing as

Log file "debug.log" content for test "functional-parallel-118-selftests/functional/basic.py:RunnerOperationTest.test_store_logging_stream_external" (FAIL):
[stdout] ======================================================================
[stdout] FAIL: test_store_logging_stream_external (basic.RunnerOperationTest)
[stdout] :avocado: dependency={"type": "ansible-module", "uri": "pip", "name": "matplotlib"}
functional-parallel-118-selftests/functional/basic.py:RunnerOperationTest.test_store_logging_stream_external: FAIL
[stdout] ----------------------------------------------------------------------
[stdout] Traceback (most recent call last):
[stdout]   File "/home/runner/work/avocado/avocado/selftests/functional/basic.py", line 848, in test_store_logging_stream_external
[stdout]     check_matplotlib_logs(os.path.join(log_dir, "full.log"))
[stdout]   File "/home/runner/work/avocado/avocado/selftests/functional/basic.py", line 833, in check_matplotlib_logs
[stdout]     self.assertIn("matplotlib __init__         L0337 DEBUG|", stream)
[stdout] AssertionError: 'matplotlib __init__         L0337 DEBUG|' not found in '2024-05-16 07:16:47,863 avocado.app human            L0064 INFO | JOB ID     : 

that nothing to do with this patch i feel , looking to fix it but meanwhile can please review this patch and if looks good merge it as this is blocker to misc-test

[richtja]

@richtja pushed changes , but CI is failing as

Log file "debug.log" content for test "functional-parallel-118-selftests/functional/basic.py:RunnerOperationTest.test_store_logging_stream_external" (FAIL):
[stdout] ======================================================================
[stdout] FAIL: test_store_logging_stream_external (basic.RunnerOperationTest)
[stdout] :avocado: dependency={"type": "ansible-module", "uri": "pip", "name": "matplotlib"}
functional-parallel-118-selftests/functional/basic.py:RunnerOperationTest.test_store_logging_stream_external: FAIL
[stdout] ----------------------------------------------------------------------
[stdout] Traceback (most recent call last):
[stdout]   File "/home/runner/work/avocado/avocado/selftests/functional/basic.py", line 848, in test_store_logging_stream_external
[stdout]     check_matplotlib_logs(os.path.join(log_dir, "full.log"))
[stdout]   File "/home/runner/work/avocado/avocado/selftests/functional/basic.py", line 833, in check_matplotlib_logs
[stdout]     self.assertIn("matplotlib __init__         L0337 DEBUG|", stream)
[stdout] AssertionError: 'matplotlib __init__         L0337 DEBUG|' not found in '2024-05-16 07:16:47,863 avocado.app human            L0064 INFO | JOB ID     : 

that nothing to do with this patch i feel , looking to fix it but meanwhile can please review this patch and if looks good merge it as this is blocker to misc-test

Hi @PraveenPenguin yes this looks like an issue of the CI, I will look at that. And about the review, thank you for your changes, I just would like to get opinion from @clebergnu

[PraveenPenguin]

@clebergnu can you please put your thought on this, as we are blocked on misc test side

[clebergnu]

Hi @PraveenPenguin ,

I tried to approach this "Test API" change with an open mind, but I'm still not convinced that the change makes sense. The use case, clearly does make sense, but I can not see why it should be part of that API.

If you look at the other parameters, they are not things that can be done outside of the responsibility of the method. In the case of converting non-absolute paths to absolute paths, that can be easily done outside of it.

For instance, what would happen if that API had the behavior of returning absolute paths, and one user wanted relative paths instead? IMO, it would be natural to simply use os.path.relpath() instead of changing the API.

If you have some other reasons or points I'm missing, please let me know.

[PraveenPenguin]

Hi @PraveenPenguin ,

I tried to approach this "Test API" change with an open mind, but I'm still not convinced that the change makes sense. The use case, clearly does make sense, but I can not see why it should be part of that API.

If you look at the other parameters, they are not things that can be done outside of the responsibility of the method. In the case of converting non-absolute paths to absolute paths, that can be easily done outside of it.

For instance, what would happen if that API had the behavior of returning absolute paths, and one user wanted relative paths instead? IMO, it would be natural to simply use os.path.relpath() instead of changing the API.

If you have some other reasons or points I'm missing, please let me know.

@clebergnu Sure, it looks ok but why this sudden change even though I am not able to get commit which yields this as earlier it was working