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

pytest: test model.Break and model.BreakQueue #641

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

deltragon
Copy link
Collaborator

@deltragon deltragon commented Aug 22, 2024

Description

CC #589

Based on #639, only the last two commits are new.

This PR adds tests using pytest for the safeeyes.model.Break and safeeyes.model.BreakQueue classes.
This is a draft, both because #639 needs to be merged first, and because this is missing a Github Action to run the tests as well as nox/tox integration.

@deltragon
Copy link
Collaborator Author

@nifadyev Do you have experience using tox/nox? Can you give some comments on how the integration should look like?

@deltragon
Copy link
Collaborator Author

deltragon commented Aug 25, 2024

This PR now also adds a test for SafeEyesCore!
It is a giant hack.
However, it did allow for me to add a test for #640 (comment) - the test code itself looks quite clean to me, if I'm honest.
It's just the support code, which has to mock threading and time pretty heavily to allow fine control over the state machine inside SafeEyesCore, which is... dark arts.

@nifadyev
Copy link
Contributor

Hey @deltragon , I do have some experience with nox, actually.

For local usage you can use pyenv to load necessary python versions and nox command - poetry run nox or just nox inside venv

Below is Github Action example with python matrix. It runs commands using specified python version, same as nox
Minor note - it is preferable to group dependencies and install only necessary ones to speed up Github Actions

name: pr

on:
  - pull_request

permissions:
  contents: read
  pull-requests: read
  checks: write

concurrency:
  group: ${{ github.workflow }}-${{ github.ref }}
  cancel-in-progress: true

env:
  PYTHON_VERSION: "3.10"
  POETRY_VERSION: "1.8.3"

jobs:
  test:
    runs-on: ubuntu-latest
    strategy:
      matrix:
        python: [ "3.10", "3.11", "3.12"]

    steps:
      - uses: actions/checkout@v4

      - name: Setup Python
        uses: actions/setup-python@v5
        with:
          python-version: ${{ matrix.python }}
          cache: 'pip'

      - name: Install Poetry
        run: pip install poetry==${{ env.POETRY_VERSION }}

      - name: Restore dependencies from cache
        uses: actions/cache@v4
        with:
          path: ~/.cache/pypoetry
          key: dependencies-cache-${{ runner.os }}-${{ env.PYTHON_VERSION }}-${{ env.POETRY_VERSION }}
          restore-keys: |
            dependencies-cache-${{ runner.os }}-

      - name: Install dependencies
        if: steps.setup-python.outputs.cache-hit != 'true'
        run: |
          poetry config virtualenvs.create false
          poetry install --no-root --no-interaction

      - name: Run Pytest on Python ${{ matrix.python }}
        run: poetry run pytest

README.md Outdated Show resolved Hide resolved

class TestSafeEyesCore:
@pytest.fixture(autouse=True)
def set_time(self, time_machine):
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch with time_machine pytest plugin but it would be better to add it as dev/test dependency with pinned version

def set_time(self, time_machine):
time_machine.move_to(datetime.datetime.fromisoformat("2024-08-25T13:00:00+00:00"), tick=False)

@pytest.fixture(autouse=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

It is advised to store fixtures in tests/conftest.py file, especially such huge fixtures

monkeypatch.setattr(model, "_", lambda message: "translated!: " + message, raising=False)

@pytest.fixture
def sequential_threading(self, monkeypatch, time_machine):
Copy link
Contributor

Choose a reason for hiding this comment

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

To tell the truth, it is quite difficult to understand. Maybe move this to separate PR? It won't make reading easy but the PR scope will be more specific

"postpone_duration": 5,
}
safe_eyes_core = core.SafeEyesCore(context)
safe_eyes_core.initialize(config)
Copy link
Contributor

Choose a reason for hiding this comment

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

Each test should assert something

def test_break_short(self):
b = model.Break(
model.BreakType.SHORT_BREAK,
"test break",
Copy link
Contributor

Choose a reason for hiding this comment

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

It is advised to use named argument to improve readability. For example, None could be anything, but image=None leaves no questions.

setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@nifadyev
Copy link
Contributor

@deltragon , great job
PR needs some improvements and simplification but even now it is super useful to have some kind of tests. Keep going)

@deltragon
Copy link
Collaborator Author

@nifadyev Thanks for the review!
Multiple of these changes come from #639 - please review that one first. I have moved your reviews over to that PR, and will reply there.
I also think I will split out and the complicated ones for SafeEyesCore into a separate PR, and keep this one only for the infrastructure and the simple tests for BreakQueue/Break.

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