Skip to content

Add test coverage for intern_atom and atom cache lifecycle#4

Merged
jholveck merged 4 commits intofeat-xcb-better-monitorsfrom
copilot/sub-pr-2-again
Feb 20, 2026
Merged

Add test coverage for intern_atom and atom cache lifecycle#4
jholveck merged 4 commits intofeat-xcb-better-monitorsfrom
copilot/sub-pr-2-again

Conversation

Copy link

Copilot AI commented Feb 20, 2026

  • Understand intern_atom function and atom caching mechanism in xcb.py
  • Add tests for predefined atoms (no XCB call) — mock xcb_intern_atom and assert_not_called
  • Add tests for cache hits/misses — real connection, selective xcb_intern_atom mock
  • Add tests for connect/disconnect updating _ATOM_CACHE — real connection, no mocking
  • Add tests for only_if_exists parameter behavior — real X server
  • Refactor into TestInternAtom class with shared autouse fixture using real xcb.connect()
  • test_atom_cache_lifecycle uses real connections, no mocking at all
  • Only LIB.xcb.xcb_intern_atom is mocked (not the whole LIB) where control/spying is needed
  • Code review and security check passed

🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

Co-authored-by: jholveck <240807535+jholveck@users.noreply.github.com>
Copilot AI changed the title [WIP] Update monitors documentation for Linux based on feedback Add test coverage for intern_atom and atom cache lifecycle Feb 20, 2026
Copilot AI requested a review from jholveck February 20, 2026 08:24
Copy link
Owner

@jholveck jholveck left a comment

Choose a reason for hiding this comment

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

It's ok, but that's a lot of code. Can we compress and combine these some? Some of the repeated code could also go into methods on a test class. BTW, there's a running X server (typically Xvfb) in the usual test environment, so we can talk to it if needed, but we don't have to.

@jholveck jholveck self-requested a review February 20, 2026 09:01
Co-authored-by: jholveck <240807535+jholveck@users.noreply.github.com>
Copy link
Author

Copilot AI commented Feb 20, 2026

It's ok, but that's a lot of code. Can we compress and combine these some? Some of the repeated code could also go into methods on a test class. BTW, there's a running X server (typically Xvfb) in ...

Refactored in 415fcde. The 9 standalone functions + helper are replaced by a TestInternAtom class with a shared setup_intern_atom autouse fixture that creates the connection, cache entry, mock, and LIB patch once per test. _set_atom_value() handles any per-test atom value changes. The connect/disconnect tests are merged into a single test_atom_cache_lifecycle function.

monkeypatch.setattr(xcb, "initialize", lambda: None)

assert cache_key not in xcb._ATOM_CACHE
xcb.connect()
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a point to mocking the connection? Can we just get the cache keys, create a new real connection, verify its address had been added to the cache keys, close the connection, and verify it was deleted?

Copy link
Author

Choose a reason for hiding this comment

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

Done in 362c124. test_atom_cache_lifecycle now snapshots the existing cache keys, calls xcb.connect() to get a real connection, asserts the new key appeared with an empty dict, calls xcb.disconnect(), and asserts the key is gone and the cache is back to its original state.

"""Tests for xcb.intern_atom and the _ATOM_CACHE mechanism."""

@pytest.fixture(autouse=True)
def setup_intern_atom(self, monkeypatch: pytest.MonkeyPatch) -> Generator[None, None, None]:
Copy link
Owner

Choose a reason for hiding this comment

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

Most of our test code tests against a real X server; you can assume that a real X server (like Xvfb) is available in the CI/CD test environment. Is there anything that we're testing that requires a completely mocked connection? We might need to monkey-patch out xcb.LIB.xcb_intern_atom with a mock for test_cache_miss_calls_xcb_and_caches_result, if that can be done.

Copy link
Author

Choose a reason for hiding this comment

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

Done in 362c124. The setup_intern_atom fixture now calls xcb.connect()/xcb.disconnect() for a real connection. Only LIB.xcb.xcb_intern_atom is selectively patched (via _mock_xcb_intern_atom) in the tests that need to spy on or control that call. test_only_if_exists_returns_none_when_missing and test_pointer_connection_uses_correct_cache_key talk directly to the real X server.

Co-authored-by: jholveck <240807535+jholveck@users.noreply.github.com>
@jholveck jholveck marked this pull request as ready for review February 20, 2026 09:35
@jholveck jholveck merged commit 7e373a3 into feat-xcb-better-monitors Feb 20, 2026
@jholveck jholveck deleted the copilot/sub-pr-2-again branch February 20, 2026 09:36
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.

2 participants