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

Feat/adding unittests for k8s prereq module #53

Merged
merged 14 commits into from
Sep 10, 2024

Conversation

boekhorstb1
Copy link
Contributor

  • added unittests: note that these only run with pytest currently. Try: .venv/bin/python -m pytest tests/modules/test_k8s_prerequisites_check.py
  • when running with unittest there are various import errors. Try: /app/rookify/.venv/bin/python -m unittest /app/rookify/src/tests/modules/test_k8s_prerequisites_check.py, also note that these import errors do not occure when testing ExampleHandler module, yet import is done in the same way. Try: /app/rookify/.venv/bin/python -m unittest /app/rookify/src/tests/modules/test_example.py
  • because the ModuleHandler loads all the module, I found that I had to mock the class TestK8sPrerequisitesCheckHandler itself, thereby keping the code for preflight method the same... maybe you know another way to mock the ModuleHandler before it gets loaded?

@boekhorstb1 boekhorstb1 self-assigned this Jun 11, 2024
Base automatically changed from prs/add-k8s-prerequisites-check to main June 12, 2024 07:09
@boekhorstb1 boekhorstb1 force-pushed the feat/adding-unittests-for-k8s-prereq-module branch 6 times, most recently from 1149441 to eda70d8 Compare June 12, 2024 08:54
@boekhorstb1 boekhorstb1 marked this pull request as ready for review June 12, 2024 09:02
Copy link
Contributor

@NotTheEvilOne NotTheEvilOne left a comment

Choose a reason for hiding this comment

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

pytest is required for unit testing. pyproject.toml contains configuration to make it work as expected which unittest to not support (even by command line argument). Later is at least true for 3.12.3 of Python at the time of writing.

load_modules() is not mockable at the moment as it scans through the rookify.modules directory to identify modules to be loaded.

@boekhorstb1 boekhorstb1 marked this pull request as draft September 4, 2024 07:55
@boekhorstb1
Copy link
Contributor Author

forgot to merge this, now have to repair conflicts :)

@boekhorstb1 boekhorstb1 force-pushed the feat/adding-unittests-for-k8s-prereq-module branch from 79fbc90 to b758793 Compare September 9, 2024 08:06
fix: adding 'object' to module path in order to use correct class
definition, allows to remove ignore comments for mypy

Signed-off-by: Rafael te Boekhorst <[email protected]>
@boekhorstb1 boekhorstb1 force-pushed the feat/adding-unittests-for-k8s-prereq-module branch from b758793 to 0d69561 Compare September 9, 2024 08:29
@boekhorstb1 boekhorstb1 marked this pull request as ready for review September 9, 2024 08:30
@boekhorstb1 boekhorstb1 force-pushed the feat/adding-unittests-for-k8s-prereq-module branch from 296aafd to 85503cc Compare September 9, 2024 10:05
@boekhorstb1 boekhorstb1 force-pushed the feat/adding-unittests-for-k8s-prereq-module branch from 3bea674 to 28f5dfd Compare September 9, 2024 10:34
@boekhorstb1
Copy link
Contributor Author

ok repaired it and added another small unittest

@boekhorstb1 boekhorstb1 force-pushed the feat/adding-unittests-for-k8s-prereq-module branch from c728caa to 3a848d2 Compare September 9, 2024 13:36
@boekhorstb1 boekhorstb1 force-pushed the feat/adding-unittests-for-k8s-prereq-module branch from 3a848d2 to 4b7a6e9 Compare September 9, 2024 14:23
@boekhorstb1
Copy link
Contributor Author

Ok I noted an issue: there seems to be some discrepancy between the local pre-comit and the pre-commit run in github actions. I now ran git commit --no-verify to avoid the local pre-commit throwing an error: github actions seems to run through everything fine though

Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
tests/modules/test_k8s_prerequisites_check.py Outdated Show resolved Hide resolved
tests/modules/test_k8s_prerequisites_check.py Outdated Show resolved Hide resolved
tests/modules/test_k8s_prerequisites_check.py Outdated Show resolved Hide resolved
@boekhorstb1 boekhorstb1 force-pushed the feat/adding-unittests-for-k8s-prereq-module branch from cf71ded to eb5ae42 Compare September 10, 2024 09:24
@NotTheEvilOne NotTheEvilOne force-pushed the feat/adding-unittests-for-k8s-prereq-module branch from 51a5aff to dad25ec Compare September 10, 2024 18:46
@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@NotTheEvilOne NotTheEvilOne force-pushed the feat/adding-unittests-for-k8s-prereq-module branch 4 times, most recently from 51f33b4 to ac7d089 Compare September 10, 2024 19:11
Signed-off-by: Tobias Wolf <[email protected]>
Modules may only implement either `preflight()` or `execute()` as well. Remove the `@abstractmethod` decorator therefore.

Signed-off-by: Tobias Wolf <[email protected]>
@NotTheEvilOne NotTheEvilOne force-pushed the feat/adding-unittests-for-k8s-prereq-module branch 6 times, most recently from 4912566 to 5c3852f Compare September 10, 2024 19:26
@NotTheEvilOne NotTheEvilOne force-pushed the feat/adding-unittests-for-k8s-prereq-module branch from 5c3852f to dde5d60 Compare September 10, 2024 19:29
Copy link
Contributor

@NotTheEvilOne NotTheEvilOne left a comment

Choose a reason for hiding this comment

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

LGTM

@NotTheEvilOne NotTheEvilOne merged commit eca2435 into main Sep 10, 2024
8 checks passed
@NotTheEvilOne NotTheEvilOne deleted the feat/adding-unittests-for-k8s-prereq-module branch September 10, 2024 19:39
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