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

Cloudinit coverage #6123

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

richtja
Copy link
Contributor

@richtja richtja commented Feb 14, 2025

This is a set of tests for avocado.utils.cloudinit utility, with fixes found on the way, to prepare it for migration to Autils and increase its coverage.

@richtja richtja added this to the 110 - Codename TBD milestone Feb 14, 2025
@richtja richtja requested a review from clebergnu February 14, 2025 15:32
@richtja richtja self-assigned this Feb 14, 2025
Copy link

codecov bot commented Feb 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.95%. Comparing base (f986a03) to head (5436765).
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6123      +/-   ##
==========================================
+ Coverage   68.92%   68.95%   +0.02%     
==========================================
  Files         203      203              
  Lines       21999    22001       +2     
==========================================
+ Hits        15163    15170       +7     
+ Misses       6836     6831       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@richtja richtja marked this pull request as draft February 14, 2025 15:52
@richtja richtja marked this pull request as ready for review February 14, 2025 16:10
Copy link
Contributor

@clebergnu clebergnu left a comment

Choose a reason for hiding this comment

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

Hi @richtja, it's great to see this utility coverage increasing and the same time marking its time to be migrated to autils.

I have a few questions and points that you can answer/address.

Thanks!

def test_phone_home_set_up(self):
sock = socket.socket()
sock.bind((self.ADDRESS, 0))
port = sock.getsockname()[1]
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC, the purpose of the previous these first 3 lines is to obtain an unused port. Once the next line (sock.close()) runs, anyone else can grab that port. Is there a reson for not allowing the PhoneHomeServer to get a port of its own, and then once it's up and running, use the port it actually acquired?

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, this is for obtaining an unused port. The reason I can't get it from PhoneHomeServer is that this test tests PhoneHomeServer.set_up_and_wait_for_phone_home which is a class method which doesn't support access to the PhoneHomeServer instance. But maybe I am missing something here.

@clebergnu clebergnu changed the title Cloudinin coverage Cloudinit coverage Feb 17, 2025
The username can be only str. Let's update the tests to respect that.

Signed-off-by: Jan Richter <[email protected]>
Since python 3.3 headers are stored to an internal buffer and
end_headers() needs to be called explicitly. This can lead to
ConnectionError.

Reference:
https://docs.python.org/3/library/http.server.html#http.server.BaseHTTPRequestHandler.send_response
Signed-off-by: Jan Richter <[email protected]>
@richtja
Copy link
Contributor Author

richtja commented Feb 19, 2025

Hi @clebergnu, I updated this PR, based on our discussion. Please take a look. I have kept the unused port obtain since I don't have a better solution, but If you have some idea, I don't mind updating it.

@richtja richtja requested a review from clebergnu February 19, 2025 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Review Requested
Development

Successfully merging this pull request may close these issues.

2 participants