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

Refactor dev (setup) #586

Merged
merged 66 commits into from
Aug 26, 2023
Merged

Refactor dev (setup) #586

merged 66 commits into from
Aug 26, 2023

Conversation

m6121
Copy link
Member

@m6121 m6121 commented Feb 22, 2023

This PR serves as a collection of some minor improvements to the development (setup). In particular it introduces the following changes:

  • Change development documentation of dumping test data according to the discussion at Extended dev-docs: dumping new test data #432
  • Extend development documentation by fake smtp server example
  • Add Github Actions check for new migrations files (./manage.py makemigrations -> no changes detected)
  • Add Github Actions check for compilemessages
  • Add pre-commit hook configuration
  • Add RDMO-Version and possibly other relevant installation information to Github Bug template (and possibly also for feature requests?)

@coveralls
Copy link

Coverage Status

Coverage: 90.462%. Remained the same when pulling 5dee438 on dev-setup-refactoring into 28809d6 on dev.

@jochenklar
Copy link
Member

Hey @m6121 @MyPyDavid @triole , I pushed my setup.cfg setup. Basically everything moves to setup.cfg but the VERSION.

@jochenklar jochenklar changed the base branch from dev-1.9.2 to dev-1.10.0 March 9, 2023 17:28
@MyPyDavid
Copy link
Member

MyPyDavid commented Apr 18, 2023

After installing https://pre-commit.com/#install
The next commits on this branch can be made with:

 SKIP=flake8,black,blacken-docs,trailing-whitespace,end-of-file-fixer git commit -m ...

# to see the pre-commit changes on all files
pre-commit run --all-files

so that all of the pre-commit auto-fixes are skipped 😉 , otherwise there 600+ files are changed

@MyPyDavid
Copy link
Member

our workflows run double on a push to a PR, Ive found this https://github.com/orgs/community/discussions/26276 and added it here.

.. but I see now it didnt run on this commit..

@jochenklar
Copy link
Member

Good idea, I would not run it on branches (except master) at all. If you want tests, create a PR first.

@jochenklar
Copy link
Member

I just tried this with another repo, and now I think it would be much better to only run tests on push and not on PR or tags. It is just super weird when you try to fix some tests and nothing is happening. Also the workflow to git commit --amend and force push does not really work with this aproach.

@afuetterer

This comment was marked as resolved.

@jochenklar
Copy link
Member

Hi @afuetterer , sadly pytoml is only supported from Python 3.10 on, which is to high for us. Seperating pytest stuff from the actual rdmo dependecies makes sense. When the dependencies are in setup.cfg, we don't need them in requirements.txt anyway. Nowadays, I usually create package like here https://github.com/ISI-MIP/isimip-publisher.

@afuetterer

This comment was marked as resolved.

@MyPyDavid
Copy link
Member

thanks @afuetterer, think this is really nice! I've tested it locally with pip install -e . and it works fine for me, also the dependencies are neat like that.

@jochenklar
Copy link
Member

Thanks again @afuetterer , I tested it with Python 3.7 and it works fine (without MANIFEST.in, pytest.ini, requirements.txt and setup.py). It would be great if you can create a PR (into this branch). I would also add/change

[project.optional-dependencies]
mysql = ["mysqlclient"]
postgres = ["psycopg2-binary"]
pytest = [
    "pytest~=7.4.0",
    "pytest-cov~=4.1.0",
    "pytest-django~=4.5.2",
    "pytest-dotenv~=0.5.2"
]

and

[tool.pytest.ini_options]
DJANGO_SETTINGS_MODULE = "config.settings"
testpaths = ["rdmo"]
python_files = "test_*.py"
pythonpath = "testing/"

@afuetterer

This comment was marked as resolved.

@afuetterer
Copy link
Member

afuetterer commented Aug 1, 2023

Hi @jochenklar, could you point this PR to another branch (dev-1.10.0 is already merged) and rebase?

@afuetterer

This comment was marked as resolved.

@afuetterer

This comment was marked as resolved.

@afuetterer

This comment was marked as resolved.

@MyPyDavid
Copy link
Member

What needs to be done here? I can work on this PR again next week.

There are some failing test cases. I can look at those.

I will look into the failing tests tomorrow, some of the tests are not correct and need to be rewritten.
They weren't failing before somehow, maybe the new pytest version handles url caching differently

Anything else? What about the check boxes in the PR description?

@MyPyDavid
Copy link
Member

I could sort of randomly reproduce the failing tests locally, there were only NoReverseMatch exceptions..
Ive added some teardowns to try clear_url_caches() and reload the django.conf settings but apparantly it didnt fix it...

@jochenklar jochenklar marked this pull request as ready for review August 25, 2023 16:09
@jochenklar
Copy link
Member

This branch is old enough, I think we accomplished most of what we wanted. I will run ruff on all files, and merge later today or in the next days.

@afuetterer
Copy link
Member

Yay for fixing the failing tests.

@afuetterer
Copy link
Member

One question though:

What is the rationale behind testing the different db backends? The Django project does extensive testing of the backends already. They shoul all work, right?

Wouldnt it maybe be enough to e.g. test only pgsql?

@MyPyDavid
Copy link
Member

MyPyDavid commented Aug 25, 2023

thanks for fixing the tests!

One question though:

What is the rationale behind testing the different db backends? The Django project does extensive testing of the backends already. They shoul all work, right?

Wouldnt it maybe be enough to e.g. test only pgsql?

No, we have had a bug where it mattered if the the db backend was mysql, sqlite or postgres. Only thing that was missing was the actual test for it ;) ... The same django queries on different dbs can yield different results..

@jochenklar
Copy link
Member

@afuetterer I think I like to test both mysql and postgres since there are always subtile differences between the backends. But I am also not sure if we will catch all those with tests.

@afuetterer
Copy link
Member

As sqlite is not recommended for production anyway, could these tests be removed then?
Testing mysql and psql should be enough?

@jochenklar jochenklar merged commit 53ca243 into dev-2.0.0 Aug 26, 2023
10 checks passed
@jochenklar jochenklar deleted the dev-setup-refactoring branch August 26, 2023 10:09
CalamityC pushed a commit to CalamityC/rdmo that referenced this pull request Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants