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

Introduce Testing #233

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Introduce Testing #233

wants to merge 9 commits into from

Conversation

merydian
Copy link
Collaborator

This branch may introduce testing.

Contents on the common and utils directories undergo unit-tests. Contents of gui are testet with an integration test that utilizes a mocked QGis interface. Each of the plugins' processing algorithms is integration tested, directly testing their respective processAlgorithm method.

Locally tests can be run using an anaconda setup, the process of which is described in the Readme file.
On Github Actions, the QGis versions 3.16, 3.22 and the latest versions are testes on, using the docker images from QGis itself.

@merydian merydian requested a review from koebi March 25, 2024 10:28
@merydian merydian marked this pull request as ready for review April 19, 2024 10:05
Copy link
Collaborator

@koebi koebi left a comment

Choose a reason for hiding this comment

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

Hey,
looks good so far 👍
My main issue is regarding the setup (conda + mamba + pip is too diverse), and one issue with the test itself.

tests/conftest.py Show resolved Hide resolved
Comment on lines +98 to +100
self.assertAlmostEqual(
response["features"][0]["geometry"], test_response["features"][0]["geometry"]
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

unittest.assertAlmostEqual() expects two integers, not two dictionaries.

Comment on lines +139 to +150
```shell
# create environment
mamba create --name qgis_test
# activate envotonment
conda activate qgis_test
# install pip
mamba install qgis pip
```
3. Install QGis using mamba.
```shell
mamba install -c conda-forge qgis=[3.16, 3.22, latest] # choose one
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be great if we could make this work with "standard" python modules.
Something like python -m venv qgis_test, source qgis_test/bin/activate should be enough here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem is that the QGis package is only available in conda, and thats what we need to run the tests. We can get rid of mamba, but its much faster and if you use the miniforge installer it ships right with it. You can only use pip inside mamba, but not the other way around.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At least when you want to run the tests on all three QGis vesions or youre on Windows.

Comment on lines +152 to +156
4. Install *xvfb*
```shell
sudo apt-get update
sudo apt install xvfb
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is xvfb needed on every system?
For me, I don't think it does anything?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think so, yes. Mostly when you run a Qt application without an actual interface.

Comment on lines +158 to +161
5. Install *Pytest* using pip in testing environment.
```shell
pip install -U pytest
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

As mentioned before, we also need pyyaml. Let's put all that in a requirements.txt file, so python -m pip install -r requirements.txt suffices

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, the only thing is that we can't put qgis itself in there. Except we switch to anaconda entirely.

Comment on lines +6 to +7
with open("ORStools/config.yml", "r+") as file:
data = yaml.safe_load(file)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this done globally?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Both functions need access to it. Is there a better way?

requirements.txt Outdated Show resolved Hide resolved
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.

None yet

2 participants