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

Improved test setup #960

Merged
merged 28 commits into from
Oct 16, 2024
Merged

Improved test setup #960

merged 28 commits into from
Oct 16, 2024

Conversation

sezanzeb
Copy link
Owner

@sezanzeb sezanzeb commented Sep 28, 2024

Applying a few things I learned in the past 2 years to input-remapper.

  • Using patch.object instead of monkey-patches
  • Moving stuff towards a dependency injection architecture, which allows better patches in tests. New classes, which might eventually be injected as dependencies:
    • PathUtils
    • UserUtils
    • Migrations
  • Patching UInput class methods, instead of patching the UInput class as a whole
  • Wrapped input-remapper-control in a class
  • Moved static logging methods into logger class

With this, import order and patch order doesn't matter anymore

This allows:

  • to make a decorator, that is applied to each test-class, to handily do the patches whenever we want them. To automatically and fully reset the patches after our tests, if we don't want them for whatever reason in other tests. And re-apply them arbitrarily later on. Using a decorator, because patches are commonly applied as a decorator already (@patch.object(...)) and it allows to do things without having to call super() in setUp and tearDown.
  • to remove the old complicated test setup (which was, funnily, already a lot better than the older one)
  • to more easily switch to pytest, if we want to.
  • Tests didn't always work out-of-the-box in IDEs, now they do.
  • using patch.object adds handy assertions that we can use on patched methods.

tests/legacy_test_setup.py Outdated Show resolved Hide resolved
tests/new_test.py Outdated Show resolved Hide resolved
inputremapper/configs/migrations.py Show resolved Hide resolved
inputremapper/configs/migrations.py Show resolved Hide resolved
inputremapper/configs/migrations.py Show resolved Hide resolved
inputremapper/configs/migrations.py Show resolved Hide resolved
inputremapper/configs/migrations.py Show resolved Hide resolved
inputremapper/configs/migrations.py Show resolved Hide resolved
inputremapper/configs/migrations.py Show resolved Hide resolved
inputremapper/configs/migrations.py Show resolved Hide resolved
inputremapper/configs/migrations.py Show resolved Hide resolved
inputremapper/configs/migrations.py Show resolved Hide resolved
@sezanzeb sezanzeb requested a review from jonasBoss October 2, 2024 09:25
@sezanzeb
Copy link
Owner Author

sezanzeb commented Oct 2, 2024

@jonasBoss this is quite a lot to review I guess, sorry. This isn't urgent, but if you don't have time to spare any time soon I feel fine with just merging it. I'm not sure if there is anything important to look at, no features were added or bugs fixed, it's just refactoring and changes to tests.

The UserUtils and PathUtils classes are a bit funky, but eventually I hope it might turn into a neat dependency-injected architecture, for which this is the first step. They are just utils mostly, but they do use some variables that need to be different during tests, so there is a point in wrapping them in a class that can be patched, which is a bit more difficult if they are globals.

@sezanzeb sezanzeb marked this pull request as ready for review October 2, 2024 09:46
@sezanzeb
Copy link
Owner Author

sezanzeb commented Oct 2, 2024

There are lots of new TODO comments scattered about mentioning dependency injection.

A case-sensitive search for ^[a-z]\w+\s=\s[A-Z].+\( finds global things that should be injected instead.

@sezanzeb
Copy link
Owner Author

sezanzeb commented Oct 2, 2024

image

😂

@sezanzeb sezanzeb mentioned this pull request Oct 2, 2024
@sezanzeb
Copy link
Owner Author

@jonasBoss are you currently available?

@jonasBoss
Copy link
Collaborator

jonasBoss commented Oct 14, 2024

yeah I currently have some free time.
Going to look at this tomorrow

Copy link
Collaborator

@jonasBoss jonasBoss left a comment

Choose a reason for hiding this comment

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

So this is mostly about moving all the standalone functions into classes?

I don't see any obvious issues!

tests/integration/test_daemon.py Outdated Show resolved Hide resolved
@sezanzeb sezanzeb merged commit 08226a2 into main Oct 16, 2024
6 checks passed
@sezanzeb sezanzeb mentioned this pull request Oct 16, 2024
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