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

fix: improved testing environment and fixed existing errors #35

Merged
merged 7 commits into from
Feb 29, 2024

Conversation

wfehr
Copy link
Contributor

@wfehr wfehr commented Feb 27, 2024

Setup a better testing-environment for running tests locally.
This is done via docker(-compose).

Other changes:

  • installed black for standardized code formatting
  • fixed isort errors

This changes are a good base for future test-implementation. Also some of the occuring errors are getting fixed.

@wfehr
Copy link
Contributor Author

wfehr commented Feb 27, 2024

Taking a look into implementing a test for "basic functionality" now.
Made this PR since the already done changes work independently and already improve the code-base.
-> e.g.: test-hooks/github-checks are running successfully now with this PR.

Copy link

codecov bot commented Feb 27, 2024

Codecov Report

Attention: Patch coverage is 48.52941% with 35 lines in your changes are missing coverage. Please review.

Project coverage is 62.12%. Comparing base (eebaa38) to head (a8d7759).

Files Patch % Lines
djangocms_transfer/cms_plugins.py 0.00% 15 Missing ⚠️
djangocms_transfer/forms.py 53.33% 14 Missing ⚠️
djangocms_transfer/cms_toolbars.py 0.00% 4 Missing ⚠️
djangocms_transfer/importer.py 0.00% 0 Missing and 1 partial ⚠️
djangocms_transfer/utils.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master      #35       +/-   ##
===========================================
+ Coverage   31.18%   62.12%   +30.94%     
===========================================
  Files          11       11               
  Lines         404      404               
  Branches       61       61               
===========================================
+ Hits          126      251      +125     
+ Misses        278      137      -141     
- Partials        0       16       +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wfehr
Copy link
Contributor Author

wfehr commented Feb 28, 2024

Just pushed the commits including tests to cover some basic functionality of this project.

I also included skipped tests with 8daa9b6. Those would cover more functionality of the imports but since I get errors for the import_file, I'll just leave those as skipped for now.

I think the currently "able to run" tests are a good base for future implementations / changes.

@wfehr
Copy link
Contributor Author

wfehr commented Feb 28, 2024

@fsbraun I'd put this changes up for review now.

@fsbraun
Copy link
Sponsor Member

fsbraun commented Feb 28, 2024

@wfehr Excellent work! Thank you!
Quick question: What is the docker file needed for?

@wfehr
Copy link
Contributor Author

wfehr commented Feb 28, 2024

@wfehr Excellent work! Thank you! Quick question: What is the docker file needed for?

Hope I didn't interfere anything with my foce-push now ;) I rebased the branch onto the current master.

The Dockerfile is used for running the tests locally via devtools/run-tests (integrated with 34b3bcc).
I can also move the file into devtools/ if the root directory isn't the best place for it.

With this setup you can run tests a little easier with different python-versions for example.
-> for example in the future one could change the base-image python:3.10.. for something like python:3.12 if older versions aren't needed for testing anymore.
I find testing via docker a little more convinient than meeting requirements in the local dev-system.

@@ -2,3 +2,6 @@
django-app-helper
tox
coverage
black
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

We're using ruff for linting and formatting, so we could merge this with black and the old linting setup and tackle ruff as a separate task. But just wanted to raise this here.

It's in pre-commit for django-cms itself; https://github.com/django-cms/django-cms/blob/develop-4/.pre-commit-config.yml#L41

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can also throw out the commits for "including black"/"formatting running black" if and maybe switch it with ruff for the development process?
Or what would be the workflow you'd want?

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I don't mind the code being formatted with black because the ruff formatter is about 97% the same.

The workflow/process around this area on the whole is using pre-commit hooks;

https://github.com/django-cms/django-cms/blob/develop-4/.pre-commit-config.yml

Combined with a github action that runs the linter;
https://github.com/django-cms/django-cms/blob/develop-4/.github/workflows/linters.yml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, then I'd leave that 'as is' for now.

@fsbraun
Copy link
Sponsor Member

fsbraun commented Feb 29, 2024

@wfehr I understand why you're using Docker for testing, but I would like to keep the test setup similar for all django-cms packages. Can you update the tox.ini to run without Docker? We can run the specific python versioned tests in the github actions.

- sorting inside project files
- configuration of sections (removed LIB since it is not defined as "known")
- changes for isort 5:
  - removed deprecated "-rc" (recursive is done by default now)
  - "-df" -> "--df"
- installed freezegun to fix datetime-values
…the test

- those tests need fixing in the future to cover more functionality with tests
@wfehr
Copy link
Contributor Author

wfehr commented Feb 29, 2024

@wfehr I understand why you're using Docker for testing, but I would like to keep the test setup similar for all django-cms packages. Can you update the tox.ini to run without Docker? We can run the specific python versioned tests in the github actions.

I updated the branch and removed docker from the first commit (34b3bcc => 541b434).

Copy link
Sponsor Member

@fsbraun fsbraun left a comment

Choose a reason for hiding this comment

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

This is great work! Thank you @wfehr !! 🚀

@fsbraun fsbraun changed the title implemented better testing environment and fixed existing errors fix: improved testing environment and fixed existing errors Feb 29, 2024
@fsbraun fsbraun merged commit 83890e2 into django-cms:master Feb 29, 2024
12 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.

3 participants