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 1 commit
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
25 changes: 9 additions & 16 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 Down Expand Up @@ -88,21 +87,15 @@ 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")

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!

'DEBUG:lib.github:127.0.0.8:9898 GET /test/user → 200',
'DEBUG:lib.github:127.0.0.8:9898 GET /test/user → 304',
])

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