Skip to content

further unit test speedups and cleanups #9226

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.

Sign up for GitHub

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

Merged
allisonkarlitskaya merged 3 commits into from
Jun 26, 2026
Merged

further unit test speedups and cleanups #9226

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cd53e8e
lib/github: fix up debugging output
allisonkarlitskaya Jun 24, 2026
cad81dc
lib/test_mock_server: two fixes for reentrancy
allisonkarlitskaya Jun 26, 2026
5785529
test: speed up some tests
allisonkarlitskaya Jun 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 10 additions & 24 deletions lib/github.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import logging
import os
import re
import socket
import subprocess
import time
import urllib.parse
Expand All @@ -37,6 +36,8 @@
from lib.directories import xdg_cache_home, xdg_config_home
from lib.testmap import is_valid_context

logger = logging.getLogger(__name__)

__all__ = (
'NOT_TESTED',
'NOT_TESTED_DIRECT',
Expand Down Expand Up @@ -65,20 +66,6 @@
_DT = TypeVar('_DT')


class Logger:
def __init__(self, directory: str):
hostname = socket.gethostname().split(".")[0]
month = time.strftime("%Y%m")
self.path = os.path.join(directory, f"{hostname}-{month}.log")

os.makedirs(directory, exist_ok=True)

# Yes, we open the file each time
def write(self, value: str) -> None:
with open(self.path, 'a') as f:
f.write(value)


class Response(TypedDict):
status: int
reason: str
Expand Down Expand Up @@ -150,31 +137,31 @@ def __init__(
try:
with open(xdg_config_home('cockpit-dev', 'github-token', envvar='COCKPIT_GITHUB_TOKEN_FILE')) as f:
self.token = f.read().strip()
logger.debug("github token loaded from %s", f.name)
except FileNotFoundError:
try:
with open(xdg_config_home('github-token')) as f:
self.token = f.read().strip()
logger.debug("github token loaded from %s", f.name)
except FileNotFoundError:
# fall back to GitHub's CLI token
try:
with open(xdg_config_home("gh/config.yml")) as f:
match = re.search(r'oauth_token:\s*(\S+)', f.read())
if match:
self.token = match.group(1)
logger.debug("github token loaded from %s", f.name)
else:
logger.debug("no oauth_token found in %s", f.name)

@mvollmer mvollmer Jun 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is "oauth token" the same as "github token" in this context? Why does this message say "oauth token"?

@mvollmer mvollmer Jun 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, this is saying "no oauth_token assignment in the config file", got it.

@allisonkarlitskaya allisonkarlitskaya Jun 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ya this is specifically about the case where (as a final fallback option) we raid the gh cli config and look for an existing token but fail to find it.

except FileNotFoundError:
# token not found anywhere, so only reading operations are available
pass
logger.debug("no github token found; only reading operations are available")

# default cache directory
if not cacher:
cacher = cache.Cache(xdg_cache_home('github'))

self.cache = cacher

# Create a log for debugging our GitHub access
self.log = Logger(self.cache.directory)
self.log.write("")

def config(self) -> Sequence[tuple[str, str]]:
"""Git config overrides for authenticating with this remote."""

Expand Down Expand Up @@ -249,7 +236,7 @@ def request(
# success!
break
except (ConnectionResetError, http.client.BadStatusLine, OSError, SSLEOFError) as e:
logging.warning("Transient error during GitHub request, attempt #%s: %s", retry, e)
logger.warning("Transient error during GitHub request, attempt #%s: %s", retry, e)

self.conn = None
time.sleep(2 ** retry)
Expand All @@ -259,8 +246,7 @@ def request(
heads = {}
for (header, value) in response.getheaders():
heads[header.lower()] = value
self.log.write(
f'{self.url.netloc} - - [{time.asctime()}] "{method} {resource} HTTP/1.1" {response.status} -\n')
logger.debug("%s %s %s → %d", self.url.netloc, method, resource, response.status)
return {
"status": response.status,
"reason": response.reason,
Expand Down
39 changes: 25 additions & 14 deletions lib/test_mock_server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@
import http.server
import json
import multiprocessing
from collections.abc import Mapping
import queue
from collections.abc import Mapping, Sequence
from typing import Never

from lib.aio.jsonutil import JsonValue

Expand All @@ -40,44 +42,53 @@
# which is why we can pass in mutable state but it's never modified.


class HTTPServer[T](http.server.HTTPServer):
class HTTPServer[T, Q = Never](http.server.HTTPServer):
reply_count = 0
data: T
queue: multiprocessing.Queue[Q]


class MockServer[T]:
def __init__(
self, address: tuple[str, int], handler: type[MockHandler[T]], data: T
):
class MockServer[T, Q = Never]:
def __init__(self, address: tuple[str, int], handler: type[MockHandler[T, Q]], data: T):
self.address = address
self.handler = handler
self.data = data
self.queue: multiprocessing.Queue[Q] = multiprocessing.Queue()

def run(self, ready: multiprocessing.synchronize.Event) -> None:
srv = HTTPServer[T](self.address, self.handler)
def run(self, port_queue: multiprocessing.Queue[int]) -> None:
srv = HTTPServer[T, Q](self.address, self.handler)
srv.data = self.data
ready.set()
srv.queue = self.queue
port_queue.put(srv.server_address[1])
srv.serve_forever()

def start(self) -> None:
ready = multiprocessing.Event()
self.process = multiprocessing.Process(target=self.run, args=(ready,))
port_queue: multiprocessing.Queue[int] = multiprocessing.Queue()
self.process = multiprocessing.Process(target=self.run, args=(port_queue,))
self.process.start()
ready.wait()
self.address = (self.address[0], port_queue.get())

def drain_queue(self) -> Sequence[Q]:
result: list[Q] = []
try:
while True:
result.append(self.queue.get_nowait())
except queue.Empty:
return result

def kill(self) -> None:
self.process.terminate()
self.process.join()
assert self.process.exitcode is not None


class MockHandler[T](http.server.BaseHTTPRequestHandler):
class MockHandler[T, Q = Never](http.server.BaseHTTPRequestHandler):
# This is wrong and broken and unsafe, but we kinda need to do it. We know
# that we'll only ever use this with the correct server type, but this
# information doesn't get carried through the library stack (in fact, we
# can't even be sure that .server here is even an HTTP server: it could be
# any socket server). So let's add it back. It's just tests...
server: HTTPServer[T]
server: HTTPServer[T, Q]

def replyData(self, value: str, headers: Mapping[str, str] = {}, status: int = 200) -> None:
self.send_response(status)
Expand Down
11 changes: 8 additions & 3 deletions test/test_aio.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from collections.abc import AsyncIterator, Iterator, Sequence
from pathlib import Path
from typing import Any
from unittest.mock import AsyncMock, patch

import httpx
import pytest
Expand Down Expand Up @@ -159,9 +160,13 @@ async def test_github_404(service: GitHubService, api: GitHub) -> None:

async def test_github_api_flakes(service: GitHubService, api: GitHub) -> None:
# Make sure 5xx errors and network issues get retries
service.flake([(503, 'Busy'), httpx.ConnectError('connection failed')])
service.update('x', {'a': 'b'}, etag=True)
assert await api.get('x') == {'a': 'b'}
mock_sleep = AsyncMock()
with patch('asyncio.sleep', mock_sleep):
service.flake([(503, 'Busy'), httpx.ConnectError('connection failed')])
service.update('x', {'a': 'b'}, etag=True)
assert await api.get('x') == {'a': 'b'}
# verify exponential backoff: 2^0=1s, 2^1=2s
assert mock_sleep.call_args_list == [((1,),), ((2,),)]


async def test_github_cache(service: GitHubService, api: GitHub) -> None:
Expand Down
10 changes: 5 additions & 5 deletions test/test_cache.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,27 +43,27 @@ def test_read_write(tmp_path: Path) -> None:


def test_current(tmp_path: Path) -> None:
c = cache.Cache[object](f'{tmp_path}', lag=3)
c = cache.Cache[object](f'{tmp_path}', lag=1)

@jelly jelly Jun 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Isn't this a bit of a micro-optimisation

@allisonkarlitskaya allisonkarlitskaya Jun 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This change is the single biggest improvement in test run times... there's just no need to wait that long... I nearly changed it to 0.1...


c.write("resource2", {"value": 2})
assert c.current('resource2') is True

time.sleep(2)
time.sleep(0.6)
assert c.current('resource2') is True

time.sleep(2)
time.sleep(0.6)
assert c.current('resource2') is False


def test_current_mark(tmp_path: Path) -> None:
c = cache.Cache[object](f'{tmp_path}', lag=3)
c = cache.Cache[object](f'{tmp_path}', lag=1)

assert c.current('resource') is False

c.write("resource", {"value": 1})
assert c.current('resource') is True

time.sleep(2)
time.sleep(0.5)
assert c.current('resource') is True

c.mark()
Expand Down
32 changes: 13 additions & 19 deletions test/test_github.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
# You should have received a copy of the GNU Lesser General Public License
# along with Cockpit; If not, see <http://www.gnu.org/licenses/>.

import fnmatch
import json
import shutil
import tempfile
Expand All @@ -28,7 +27,6 @@
from lib import cache, github
from lib.test_mock_server import MockHandler, MockServer

ADDRESS = ("127.0.0.8", 9898)
GITHUB_ISSUES = [{"number": "5", "state": "open", "created_at": "2011-04-22T13:33:48Z"},
{"number": "6", "state": "closed", "closed_at": "2011-04-21T13:33:48Z"},
{"number": "7", "state": "open"}]
Expand Down Expand Up @@ -70,10 +68,11 @@ def do_DELETE(self) -> None:

class TestGitHub(unittest.TestCase):
def setUp(self) -> None:
self.server = MockServer(ADDRESS, Handler, GITHUB_ISSUES)
self.server = MockServer(("127.0.0.1", 0), Handler, GITHUB_ISSUES)
self.server.start()
self.temp = tempfile.mkdtemp()
self.api = github.GitHub(f"http://{ADDRESS[0]}:{ADDRESS[1]}/", cacher=cache.Cache(self.temp))
url = f"http://{self.server.address[0]}:{self.server.address[1]}/"
self.api = github.GitHub(url, cacher=cache.Cache(self.temp))

def tearDown(self) -> None:
self.server.kill()
Expand All @@ -88,21 +87,16 @@ def test_cache(self) -> None:
self.assertEqual(count, 1)

def test_log(self) -> None:
self.api.get("/test/user")
self.api.cache.mark(time.time() + 1)
self.api.get("/test/user")

expect = (
'127.0.0.8:9898 - - * "GET /test/user HTTP/1.1" 200 -\n'
'127.0.0.8:9898 - - * "GET /test/user HTTP/1.1" 304 -\n'
)

with open(self.api.log.path, "r") as f:
data = f.read()

match = fnmatch.fnmatch(data, expect)
if not match:
self.fail(f"'{data}' did not match '{expect}'")
with self.assertLogs('lib.github', level='DEBUG') as logs:
self.api.get("/test/user")
self.api.cache.mark(time.time() + 1)
self.api.get("/test/user")

netloc = f'{self.server.address[0]}:{self.server.address[1]}'
self.assertEqual(logs.output, [

@mvollmer mvollmer Jun 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Indentation? Doesn't this need to be under the with ... as logs: statement?

@allisonkarlitskaya allisonkarlitskaya Jun 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

no. it doesn't. and maybe even shouldn't. the inside the with: is the thing under observation and the outside of it is the inspection of what happened during that observation period.

compare that also with the assertRaises for example where it's even more clear why you'd want it that way:

with self.assertRaises(ValueError) as cm:
    do_something()

self.assertEqual(str(cm.exception), "invalid value")
self.assertEqual(cm.exception.args, ("invalid value",))

because you can't do the compare until after the exception, but the exception is going to shoot you out of the block.

@mvollmer mvollmer Jun 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Aha, thanks! I was mostly suprised that "logs" is even available outside of the "with". TIL!

f'DEBUG:lib.github:{netloc} GET /test/user → 200',
f'DEBUG:lib.github:{netloc} GET /test/user → 304',
])

def test_issues_since(self) -> None:
issues = self.api.issues(since=1499838499)
Expand Down
Loading
Loading