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

Add tests to ensure that pastes are reaped properly. #239

Merged
merged 9 commits into from
Mar 2, 2024

Conversation

shtlrs
Copy link
Contributor

@shtlrs shtlrs commented Jan 14, 2024

Closes #182

Note: I tried to make Kaizen style commits, so you can read them one by one.

Structural changes

Some refactoring has been done in order to make testing easier, but also to improve the overall quality.

Database models

All db models have been moved to their own models module.

DatabaseManager

The engine instance was a global variable that everyone could access/see from the database module. This isn't quite a good approach as it's always better to have a provider for such things.
Enter the DatabaseManager: A class to handle database-related resources better, in this case: the engine and a session

This helps us hide the engine from its peers by making it available only through a provider method.
It also helps us alot when it comes to testing, since we can monkey patch the instance per scope easier.

Configuration & ConfigurationProvider

We used to have problems when it comes to getting the same configuration, imports were placed inside functions to ensure that values are bound to variables correctly.

This has been handled thanks to the ConfigurationProvider and the Configuration classes.

Configuration

This is just a class whose responsibilits is to encapsulate the configuration values we have, to make sure these values are not affected by imports thanks to late binding.

This will also make monkeypatching/mocking easier.

ConfigurationProvider

This acts as the unique source of truth of the Configuration instance by making it a Singleton, thus making it the one responsible for instantiating the configuration when needed.

Features

This PR adds support for making the reaping task's periodicity configurable.

Tests

It also adds tests that do the following

  1. Ensure that the reap task keeps running when the app has started.
  2. Ensure that expired pastes are reaped upon startup
  3. Ensure that expired pastes are reaped whenever we try to review them.

@shtlrs
Copy link
Contributor Author

shtlrs commented Jan 14, 2024

Strange, the e2es ran fine locally.

I'll investigate this tomorrow.

@shtlrs
Copy link
Contributor Author

shtlrs commented Jan 14, 2024

The tests are failing because there's a bug when it comes to loading the config file.

Expiries and ratelimites are not being overwritten.

I'll have a look and fix that.

1. This adds no maintenance cost
2. This is a nice to have feature if anyone wanted to configure it
3. This will allow us to test the task easily.
@shtlrs
Copy link
Contributor Author

shtlrs commented Jan 14, 2024

Strange, but 1de2f0b from #231 isn't present in my local repo.
I've synced my fork and fetched all remotes, but I still don't see it.
Cherry-picking fails since git doesn't have it in its local tree.

@supakeen Where do we use this gitpython package ?

EDIT

I've fixed it by not downgrading gitpython when installing pytest-asyncio.

This will allow us to execute async tests.

The reason it's needed is we want to be able to call asyncio.sleep so that we don't block the event loop while waiting for other tasks to be ran
This is part of a restructure of how database connections/engines/ are created
This ensure that we don't have global resource variables available all across modules.

The delegation will help contain the instance better, and operate more granulay on it during tests.
This makes the localization more precise
The Configuration class will help structure the configuration better and will allow us to mock values easier when testing.

The ConfigurationProvider is responsible for providing the same config across the entire app through late binding.
@supakeen supakeen merged commit 049421a into supakeen:master Mar 2, 2024
10 checks passed
@shtlrs shtlrs mentioned this pull request Mar 8, 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.

Are pastes being reaped properly?
2 participants