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

Feature/276 new script file for testing framework #238

Merged
merged 80 commits into from
Apr 2, 2025

Conversation

rolandoquesada
Copy link
Contributor

No description provided.

rolandoquesada and others added 20 commits October 23, 2024 01:58
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.
    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
@cwlacewe cwlacewe added this to the v2.11.0 Tasks milestone Nov 6, 2024
Copy link
Contributor

Target CPP Coverage: 64.2822%
Source CPP Coverage: 64.2172%

Target Python Coverage: 97.94%
Source Python Coverage: 97.94%

Copy link
Contributor

Target CPP Coverage: 64.2822%
Source CPP Coverage: 64.2172%

Target Python Coverage: 97.94%
Source 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]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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
Copy link
Contributor Author

@rolandoquesada rolandoquesada Mar 25, 2025

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cars.xml is used in the example code of the README. Please ensure that the resource files are the same throughout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @rv355 Please correct me if I am wrong. If Cars.xml file is provided, then the resources folder should not be included as part of the documentation in the README.md file because that folder contains the haarcascade_frontalface_default.xml file which for this case is not needed anymore, am I right?

```
python3 udf_server.py 5010
python3 udf_server.py 5010 [path_tmp_dir]
Copy link
Contributor

@rv355 rv355 Mar 26, 2025

Choose a reason for hiding this comment

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

The use of path_tmp_dir should also be mentioned in the Setup section.

Copy link
Contributor

Target CPP Coverage: 64.2822%
Source CPP Coverage: 64.2172%

Target Python Coverage: 97.94%
Source Python Coverage: 97.94%

Copy link
Contributor

Target CPP Coverage: 64.2822%
Source CPP Coverage: 64.2172%

Target Python Coverage: 97.94%
Source Python Coverage: 97.94%

@rolandoquesada
Copy link
Contributor Author

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.

Copy link
Contributor

@ifadams ifadams left a comment

Choose a reason for hiding this comment

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

Only real change/explanation I would at thgis point is the DB pointer in the query handler/server initialization. Other than that, I think we're good to go.

@@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the delay, had a lot of unrelated chaos last week.

If its only used in the unit-tests, it does not appear to be causing serious issues. Is there a way to limit the de-allocation to them? My only concern is that we're adding side-effects in a place thats way far away from the proximate causes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Too large to effectively review, hoping functionality is enough to show its doing what its supposed to.


cd ..

# Start server for client test
./../build/vdms -cfg unit_tests/config-tests.json > tests_screen.log 2> tests_log.log &
./../build/vdms -cfg /tmp/tests_output_dir/config-tests.json > /tmp/tests_output_dir/tests_screen.log 2> /tmp/tests_output_dir/tests_log.log &
Copy link
Contributor

Choose a reason for hiding this comment

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

This and below both output to the same log name and location, will this be a problem?

Copy link
Contributor Author

@rolandoquesada rolandoquesada Apr 2, 2025

Choose a reason for hiding this comment

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

Hi, the original run_tests.sh script file redirected the logs of the different VDMS servers to the same temporary file even when the data was different. I wanted to keep the same approach, however, I could change it to do the logging in different temporary files.
The info is being appended to the same file, so the possible issue is to try to understand which VDMS server logged that information.
@ifadams please let me know if I should keep the logic as it is right now, or if I should change it to log the data to different log files.
Thank you.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, should probably be different files. Should be a fast fix, however.

Copy link
Contributor

Choose a reason for hiding this comment

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

We will ignore the log setup for now since this was original behavior. If any issues arise, we can address later

socket = context.socket(zmq.REP)
socket.bind("tcp://*:" + str(settings["port"]))
# Function to dynamically import a module given its full path
def import_module_from_path(module_name, path):
Copy link
Contributor

Choose a reason for hiding this comment

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

@rolandoquesada assuming this has been dealt with?

@rolandoquesada rolandoquesada requested a review from ifadams April 2, 2025 07:46
Copy link
Contributor

github-actions bot commented Apr 2, 2025

Target CPP Coverage: 64.2822%
Source CPP Coverage: 64.2075%

Target Python Coverage: 97.94%
Source Python Coverage: 97.94%

@cwlacewe cwlacewe merged commit 5b6dfe2 into develop Apr 2, 2025
2 checks passed
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.

None yet

6 participants