Skip to content

Full migration to idiomatic pytest - #1225

Merged
ryneeverett merged 27 commits into
GothenburgBitFactory:developfrom
Lotram:idiomatic-pytest
Jul 20, 2026
Merged

Full migration to idiomatic pytest#1225
ryneeverett merged 27 commits into
GothenburgBitFactory:developfrom
Lotram:idiomatic-pytest

Conversation

@Lotram

@Lotram Lotram commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Last PR to close #1160

This PR mostly does 2 things:

  1. migrate everything to more standard pytest (fixtures, parametrize...)
  2. Improve consistency across service test files.

Standard Pytest

This change is quite opinionated, but here is why I think it improves the code:

  • Remove a lot of silent mutations. The tests used to rely on manual copies, setting / unsetting environment variable etc to make everything works. Tests are now isolated by construction. We use standard fixture for tmp paths, env variable overrides, and we define our own to create services, issues etc.
  • composition over inheritance. Standard principle too, we used to rely a lot on test class inheritance, which is quite fragile, and prevented us to easily modify tests (hence this big PR)
  • warnings are now considered as errors, so we can detect and fix them (except for taskw deprecation warnings, as we can't really do anything about that for now). Several bugs were detected and fixed thanks to that.

Service tests consistency

Each service test was defining and using its data differently, once again making it harder to add a change for all services at once, I tried to make them a bit more similar. I added a few tests to ensure basic consistency

I tried to create atomic commits to ease the review. If that's still too much, I can create multiple PRs

Lotram added 23 commits July 9, 2026 10:11
Move each module's SERVICE_CONFIG to module level, share the
validate-and-instantiate pipeline as get_validated_service, and derive
duplicated config fixtures from SERVICE_CONFIG.
Fake API payloads become record/extra fixtures returning fresh objects
(a data SimpleNamespace where values are interdependent), replacing the
mix of mutable class attributes and TEST_*/ARBITRARY_* constants.
Validation classes are TestXConfig, config fixtures target 'myservice',
and gmail drops its bespoke section name.
Every service test module must define a module-level SERVICE_CONFIG and
keep fake data in fixtures rather than mutable class attributes.
The test layered responses.activate on top of the class's autouse
RequestsMock, relying on nested-mock shadowing.
test_templates kept its fake payload as class attributes and deck built
services through a plain method rather than a factory fixture.
A single conftest factory reads each module's SERVICE_CLASS and
SERVICE_CONFIG, replacing the per-module copies. Modules that mock at
construction keep a local fixture built on the shared factory;
bugzilla, azuredevops, and deck override it outright.
The SimpleNamespace bundles in the gitlab, github, jira, and redmine
tests become one fixture per value, with interdependencies expressed as
fixture parameters, so all modules share the record/extra fixture shape.

@ryneeverett ryneeverett left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

  • FYI, I'm thinking I'll squash merge this one. The commit structure was helpful for review but I'm not sure how helpful it would be in a git blame scenario. I also noticed that the commits do not appear to all be atomic in the sense that the tests pass -- f825c0c removes ConfigTest which is still depended on.
  • Please update the tutorial.rst Tests section to reflect the new pattern.
  • This is more an idea for the future than something I'd want added to this PR, but would is make sense to make config a shared fixture across services? My thought is that is might be confusing that the default differs from SERVICE_CONFIG. It would require some reworking in a lot of the services, but it seems nice if they all used a fixture like Trello:
    @pytest.fixture
    def config(self):
        return {'general': {'targets': ['myservice']}, 'myservice': {**SERVICE_CONFIG}}

Comment thread tests/services/test_gerrit.py Outdated
Comment thread tests/config/test_data.py Outdated
Comment thread tests/services/test_github.py Outdated
Comment thread tests/services/test_github.py Outdated
Comment thread tests/services/test_gitlab.py
Comment thread tests/services/test_gitlab.py Outdated
Comment thread tests/services/test_jira.py Outdated
Comment thread tests/test_templates.py Outdated
Comment thread tests/test_templates.py Outdated
@Lotram

Lotram commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator Author

FYI, I'm thinking I'll squash merge this one. The commit structure was helpful for review but I'm not sure how helpful it would be in a git blame scenario. I also noticed that the commits do not appear to all be atomic in the sense that the tests pass -- f825c0c removes ConfigTest which is still depended on.

Agreed, I created multiple commits to avoid having a single really big diff, but we can definitely squash them before merging. Do you want me to do it now, or wait ?

Please update the tutorial.rst Tests section to reflect the new pattern.
Sure

This is more an idea for the future than something I'd want added to this PR, but would is make sense to make config a shared fixture across services? My thought is that is might be confusing that the default differs from SERVICE_CONFIG. It would require some reworking in a lot of the services, but it seems nice if they all used a fixture like Trello

Not sure I understand how you want to pass the SERVICE_CONFIG dict to a shared fixture. Through a factory maybe ? Something like:

@pytest.fixture
def config_factory(service_config):
    def _config():
        return {'general': {'targets': ['myservice']}, 'myservice': {**service_config}}
    return _config

# in test_github.py
SERVICE_CONFIG = {}
@pytest.fixture
def config():
    return config_factory(SERVICE_CONFIG)

@Lotram

Lotram commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator Author

@ryneeverett I added a commit to speed up the documentation tests (as well as the actual doc generation).

this package is a transitive dependency, used by ini2toml.
@ryneeverett

Copy link
Copy Markdown
Collaborator

I'll squash at the end since the github UI makes it super easy, I just wanted to get your opinion. I hadn't been squashing your PR's lately since the commits have been well-structured and truly atomic.

Not sure I understand how you want to pass the SERVICE_CONFIG dict to a shared fixture.

It seems like we could use the request fixture and do something like:

    @pytest.fixture
    def config(self, request):
        return {'general': {'targets': ['myservice']}, 'myservice': {**request.module.SERVICE_CONFIG}}

@ryneeverett
ryneeverett merged commit d539f85 into GothenburgBitFactory:develop Jul 20, 2026
7 checks passed
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.

Consider full switch to Pytest

2 participants