Move temporary dir deletion to inner function, delete temp dir in skipped benchmarks #248
Conversation
Codecov ReportPatch coverage:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #248 +/- ##
==========================================
- Coverage 56.71% 56.52% -0.19%
==========================================
Files 24 24
Lines 2234 2245 +11
==========================================
+ Hits 1267 1269 +2
- Misses 967 976 +9
☔ View full report in Codecov by Sentry. |
| @@ -912,9 +912,7 @@ def process_self_contained_coordinator_stream( | |||
| logging.critical("Printing redis container log....") | |||
| print("-" * 60) | |||
| print( | |||
| redis_container.logs( | |||
| stdout=True, stderr=True, logs=True | |||
There was a problem hiding this comment.
Also there was an issue with this line in contained_coordinator. When benchmark raised an exception for any reason we tried to print container logs but it resulted with: TypeError: ContainerApiMixin.logs() got an unexpected keyword argument 'logs'. According to https://docker-py.readthedocs.io/en/stable/containers.html#docker.models.containers.Container.logs parameter logs for method logs() doesn't exist. This led to skipping 'teardown' stage which led to temporary folders being left with the redis-server artifact inside.
There was a problem hiding this comment.
With that change instead we get:
2023-09-13 01:33:49 CRITICAL Some unexpected exception was caught during local work. Failing test....
2023-09-13 01:33:49 CRITICAL <class 'KeyError'>
2023-09-13 01:33:49 INFO Tearing down setup
2023-09-13 01:33:49 INFO When trying to stop DB container with id a9b45d05a8ba306464d79d3d0deeaa2a72dd7d8761b838aefe5d318326e1cf5e and image <Image: 'debian:buster'> it was already stopped
2023-09-13 01:33:49 INFO Removing temporary dirs /root/tmp7rhog4yu and /root/tmpkcu7fzqu
And temporary dirs are removed
There was a problem hiding this comment.
Hi @mpozniak95, thank you for PR!
Could you please fix integration test failure? This is failing - test_run_local_command_logic_oss_cluster according to log.
You can use these PRs for test's run reference:
Let me know in case of questions.
|
I think that that wasn't fault of my PR, there was an "Connection Refused" exception when trying to connect to Redis. I re-run test and it worked. |
|
@filipecosta90 , could you please check and merge if you are agree? |
Opened again from branch created in this repository instead of forked branch.
Probably connected with issue: #244.
So far when some check resulted with skipping benchmark, it would keep an empty temporary directory in home directory. Deletion of temporary dir should happen in such situation before continuing to the next benchmark.
This PR moves temporary directory deletion to inner function in process_self_contained_coordinator_stream and invokes it before skipping any benchmark