Skip to content
This repository has been archived by the owner on Nov 20, 2024. It is now read-only.

Addressing Issue: Temporary failure in name resolution #84

Merged
merged 7 commits into from
Feb 7, 2023

Conversation

edavalosanaya
Copy link
Contributor

This PR is made to address #82. It is composed of three commits:

  • First, added dotenv to the __init__.py of the g3pylib package to ensure that the G3_HOSTNAME environmental variable is loaded when the package is loaded. Not sure if this is the desired behavior, as the examples all perform this function within their mains. Having this as default would benefit the tests, as the loading of the environmental variables was not performed, causing certain tests to fail.
  • Had to ignore certain tests that would hang and freeze. I couldn't run the complete test suite with pytest. I added @pytest.mark.skip to these offending tests. Now the test suite should be able to run, yet it still has some failing tests. Also added a timeout for tests.
  • The streamgazerstp.py example felt lacking in that it would not show the gaze on the video. Just made OpenCV draw the gaze on the video.

NOTES:
Used pre-commit.

Best,
Eduardo Davalos

@edavalosanaya
Copy link
Contributor Author

Just realized that you are using pytest-dotenv plugin. Does that mean that .env needed to be store within the test folder?

if "gaze2d" in gaze:
gaze2d = gaze["gaze2d"]
logging.info(f"Gaze2d: {gaze2d[0]:9.4f},{gaze2d[1]:9.4f}")
h, w = img.shape[:2]
fix = (int(gaze2d[0] * w), int(gaze2d[1] * h))
img = cv2.circle(img.copy(), fix, 10, (0, 0, 255), 3)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice addition with the gaze overlay. Is it really necessary to copy the image? isn't it better to just draw on the original image (without setting the img-variable again)?

https://www.geeksforgeeks.org/python-opencv-cv2-circle-method/

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried locally to remove the copy and the assignment and it still worked like a charm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you are right, will make those changes.

@@ -65,3 +65,6 @@ skip_gitignore = true
[tool.pytest.ini_options]
asyncio_mode = "auto"
testpaths = ["tests"]

# Timeout
faulthandler_timeout=30
Copy link
Contributor

Choose a reason for hiding this comment

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

Another nice addition. A failed test is a better test than a non-terminating test.

@@ -9,6 +9,7 @@ async def test_get_name(g3: Glasses3):
assert await g3.recordings.get_name() == "recordings"


@pytest.mark.skip(reason="Test causes pytest to freeze")
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this test freeze even after the timeout was added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it would cause the test suite to hang and not continue.

@jonashogstrom
Copy link
Contributor

Thanks for your contribution. The gaze overlay and the timeout are nice changes and if you submit them as separate PRs I'd be happy to approve them. I need to refresh my memory on the details on the .env file and tests. stay tuned.

@jonashogstrom
Copy link
Contributor

Just realized that you are using pytest-dotenv plugin. Does that mean that .env needed to be store within the test folder?

the .env-file should be in the root folder (mentioned in the readme.md). When you have this, there is no need to call env.load_dotenv() in the main library. Since a real client application is unlikely to expect the hostname to be specified like this it is better to call dotenv.load_dotenv() in the initialization of the examples.

@edavalosanaya
Copy link
Contributor Author

Sorry for the delay, winter break + project deadline. Thank you for your helpful comments. I have removed the dotenv usage within the __init__, cleaned up the example code, and remove the modification to the test case. For some reason, I cannot run the test, likely due to an error in my network setup.

Copy link
Contributor

@jonashogstrom jonashogstrom left a comment

Choose a reason for hiding this comment

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

Looks good now. Thanks @edavalosanaya !

Copy link
Contributor

@jonashogstrom jonashogstrom left a comment

Choose a reason for hiding this comment

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

Looks good now. Thanks @edavalosanaya !

@jonashogstrom
Copy link
Contributor

There are some code formatting errors. I'll resolve them and get this fix merged.

@jonashogstrom jonashogstrom merged commit 14c7a79 into tobiipro:main Feb 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants