Feature/276 new script file for testing framework #238
Conversation
…github.com/IntelLabs/vdms into feature/276_new_script_file_for_testing_01
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Updating permissions for Results Job to add content: write to pass to CI_Coverage_compare.
Update CI.yml
Print out the log files created by the testing
Update run_all_tests.py to use python3
Attempt to check if the requirements are installed correctly
Add random pid values to those tests
There was a problem hiding this comment.
In general, but I'm seeing a LOT of updates in core code across many modules. While I understand these may have come across as fixing failing tests and just general code cleanup, the sheer number in one pull request makes me a bit nervous.
In the future lets try to keep the code volume lower per PR.
There was a problem hiding this comment.
In general I'd like to see (even if its just here in the PR) some explanations on why certain changes to core modules have been made as its way outside the initial scope of this task which was intended to be focused on breaking apart of better compartmentalizing of the testing.
| @@ -58,6 +60,10 @@ void PMGDQueryHandler::init() { | |||
| // These parameters can be loaded everytime VDMS is run. | |||
| // We need PMGD to support these as config params before we can do it here. | |||
|
|
|||
| if (_db != nullptr) { | |||
There was a problem hiding this comment.
Is this safe to do?Is _db shared anywhere? Is it valid to have multiple references to the same PMGDQueryHandler? I'm not clear on this without a fair bit of digging.
There was a problem hiding this comment.
Hi Keira, there was a pre-existing memory leak in the line 62 when the init() method was called more than one time. To avoid that I added the corresponding deallocation of the memory.
There was a problem hiding this comment.
Hi @ifadams, for avoiding double initializations, should we check if the init() has already been called before? If we should, then I guess the init() function should return a boolean value which will be true when PMGDQueryHandler was initialized successfully and false when it was already initialized or an error happened. I would like to know your opinion about it.
There was a problem hiding this comment.
Do we know where init was called elsewhere?
There was a problem hiding this comment.
Hi @ifadams, it seems that PMGDQueryHandler::init() method is called only in Server.cc file. However, in the unit tests that method is called multiple times but the memory is never deallocated by PMGDQueryHandler::destroy() method (which is causing several memory leaks).
Based on that, I'm wondering if you could please tell me how I should proceed on the code change that I added for avoiding memory leaks?
tests/udf_test/functions/files/haarcascade_frontalface_default.xml
Outdated
Show resolved
Hide resolved
Remove unused parameters in some functions
|
Target CPP Coverage: 64.2822% Target Python Coverage: 97.94% |
…github.com/IntelLabs/vdms into feature/276_new_script_file_for_testing_01
|
Target CPP Coverage: 64.2822% Target Python Coverage: 97.94% |
| @@ -38,7 +38,7 @@ source venv/bin/activate | |||
| python3 -m pip install pip --upgrade | |||
| python3 -m pip install wheel | |||
| python3 -m pip install -r requirements.txt | |||
| python3 udf_server.py <port_number> | |||
| python3 udf_server.py <port_number> [path_tmp_dir] | |||
There was a problem hiding this comment.
@rv355 I would like your opinion about this parameter that I added to the script file, I added it for letting the server know in which directory to create the temporary files.
There was a problem hiding this comment.
@rolandoquesada Looks fine to me.
| | |__facedetect.py | ||
| |__README.md | ||
| |__requirements.txt | ||
| |__udf_server.py | ||
| ``` | ||
| 2. Download/Copy the `cars.xml` file to the `~/remote_function/functions/files`. | ||
| 3. Create the `cardetect.py` file in `~/remote_function/functions`. | ||
| 2. Copy the `resources` directory (located at the root of the repo) next to the `remote_function` directory |
There was a problem hiding this comment.
@rv355 I added this documentation to the README.md file so the user can know that the resources directory (it includes the _haarcascade_frontalface_default.xml file) needs to be copied to the required location.
There was a problem hiding this comment.
Cars.xml is used in the example code of the README. Please ensure that the resource files are the same throughout.
| ``` | ||
| python3 udf_server.py 5010 | ||
| python3 udf_server.py 5010 [path_tmp_dir] |
There was a problem hiding this comment.
The use of path_tmp_dir should also be mentioned in the Setup section.
….py files" This reverts commit de9a709.
|
Target CPP Coverage: 64.2822% Target Python Coverage: 97.94% |
|
Target CPP Coverage: 64.2822% Target Python Coverage: 97.94% |
|
I fixed almost all the requested changes to the code, I am going to proceed with updating the wiki according to the suggestions given by @rv355. Thank you. |
No description provided.