Francescalb/testing dependencies #142

francescalb · 2023-10-17T13:11:10Z

Description:

Type of change:

Bug fix.
New feature.
Documentation update.
Testing.

Checklist for the reviewer:

This checklist should be used as a help for the reviewer.

Is the change limited to one issue?
Does this PR close the issue?
Is the code easy to read and understand?
Do all new feature have an accompanying new test?
Has the documentation been updated as necessary?

Removed hard requirement on DLite in tests.

This allows for testing locally and then pytest does not check global imports

Only test backends that are installed in triplestore

codecov-commenter · 2023-10-18T09:08:56Z

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (dba3971) 75.67% compared to head (3533c5b) 75.08%.
Report is 8 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #142      +/-   ##
==========================================
- Coverage   75.67%   75.08%   -0.59%     
==========================================
  Files          17       17              
  Lines        1402     1405       +3     
==========================================
- Hits         1061     1055       -6     
- Misses        341      350       +9

Files	Coverage Δ
tripper/mappings/__init__.py	`100.00% <100.00%> (ø)`
tripper/utils.py	`83.60% <ø> (-3.28%)`	⬇️
tripper/mappings/mappings.py	`66.30% <35.71%> (-1.10%)`	⬇️

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

Sparqlwrapper test skipped if package missing

tests/test_add_function.py

jesper-friis

Given that we want to tie ourselves to pytest this is a great PR that does what it intends to do and nothing more. Approves under that premises.

The bad thing is that the tests becomes less readable (or at least it require that the reader spends time that could have been used for something else to learn pytest) and makes interactive debugging with e.g. ipython impossible (because the pytest --pdb option is broken).

Another possibility would be to find an alternative test runner that doesn't load the tests without properly importing them and fails for missing packages even for test scripts that exit gracefully if depending packages are not installed.

CasperWA

The only "hard" change I'd request is the usage of v4 for the actions/checkout GitHub action in ci_tests.yml. As well as possibly adding version specifiers for pytest and pytest-cov in the same file.

Excellent work here 👍

.github/workflows/ci_tests.yml

CasperWA · 2023-10-19T07:41:46Z

.github/workflows/ci_tests.yml

+        pip install pytest
+        pip install pytest-cov


To ensure stability it would be nice to constrict these installations to a version range. This can be most easily done in three different ways, two of which ensures dependabot updates, and again one of those that would require no changes to the dependabot config:

Add version specifiers directly in this line (probably copying those already existing in pyproject.toml.
Requires_: Manually checking and updating the version specifiers later.

Create new requirements_testing_basic.txt or similar in .github/utils/ or similar directory within the .github directory. This file then includes the pytest and pytest-cov dependencies including version specifiers as given currently in pyproject.toml. This file is then installed here like: pip install -r .github/utils/requirements_testing_basic.txt, instead of these lines.
Requires: Updating the dependabot config file (at .github/dependabot.yml) to include automatic updates of the requirements file. See below this list what this addition should look like.

Add a new extra in pyproject.toml called "testing-core" or "testing-framework" or similar that only includes pytest and pytest-cov with the version specifiers already existing in the pyproject.toml under the "testing" extra. This extra is then installed here like: pip install -e .[testing-core], removing the pip install -e . line as well as these two line.
Requires: No extra work, only the change in pyproject.toml. The specifiers will be automatically checked and updated.

Here follows the addition to dependabot.yml if choice number 2 from the above list is chosen:

- package-ecosystem: pip directory: "/.github/utils" schedule: interval: weekly day: monday time: "05:18" # Should be bigger than or equal to the total number of dependencies (currently 2) open-pull-requests-limit: 5 target-branch: ci/dependency-updates labels: - CI/CD - skip_changelog

Note: If the requirements file is not put in .github/utils/ the value for directory above should be changed accordingly.

I prefer option 3

CasperWA · 2023-10-19T07:43:42Z

tests/convert/test_convert.py

+
+def test_convertions():
+    """Test convertions. Uses rdflib as triplestore backend"""
+    import pytest


I think pytest can be imported at the top-level, i.e., outside the functions at the top of the file, if so desired?

Then you can put the pytest.importorskip("rdflib") at the top of the file out side the function as well.
In this way the test will truly just contain what it's about: the test.

By putting it outside it will also ensure that these lines will not have to be repeated if new test functions should be created in this file that requires rdflib - however, if the file will contain a mix of tests that require and not require rdflib, then this approach is to be preferred.

Note, a kind reminder that all comments here are in general suggested changes, this in particular is indeed only a suggestion.

This comment holds for all other places in this PR that a similar thing is done.

I have thought about it. I really prefer to do the importorskip for each test-function. To make sure that one does not accidentally skip new function added at a later stage. It is easy to miss out on things at the top of the file.

pytest can of course be moved outside.

tests/mappings/test_mappings.py

tests/test_namespace.py

tests/test_triplestore.py

tripper/mappings/mappings.py

CasperWA · 2023-10-19T08:01:56Z

tripper/mappings/mappings.py

 from __future__ import annotations  # Support Python 3.7 (PEP 585)

 import subprocess  # nosec: B404
 from collections import defaultdict
 from enum import Enum
 from typing import TYPE_CHECKING, Mapping, Sequence

-import numpy as np
 from pint import Quantity  # remove


This is not part of this PR, so this comment can be disregarded (or a follow-up issue opened).

Why is there a # remove comment for this line?

Don't know. I'd leave it for now.

tripper/utils.py

Co-authored-by: Casper Welzel Andersen <[email protected]>

CasperWA

All good 👍

Please open follow-up issues according to the comments in my previous review.

As a tip, these can easily be created in a "quick-and-dirty"-style from a given comment:

Co-authored-by: Casper Welzel Andersen <[email protected]>

jesper-friis and others added 8 commits September 29, 2023 20:04

Clearified that tripper does not depend on DLite and Pydantic.

9fcff76

Removed hard requirement on DLite in tests.

Merge branch 'master' into clarifying-dependencies

6e06847

Added test to check if DLite is installed for test_collection

4d29686

Tried to remove errors on duplicate code in tests

da53897

add pytest to tests without dependencies

b99e26e

pytest-cov must be included

165bede

Tests for collection and utils into separate functions.

8af0728

This allows for testing locally and then pytest does not check global imports

Removed hard numpy requirements for mappings.

76f5624

Only test backends that are installed in triplestore

francescalb added 10 commits October 18, 2023 11:16

Pytest only tests the testsfolder when only basic is installed.

ccb8214

Sparqlwrapper test skipped if package missing

Update tests/test_add_function

ff8df48

Don't test convert if rdflib not installed

b9767e9

tests for mappings not run if rdflib missing

b51eb39

test_namespce and test_triplestore not tested if missing imports

46a54cc

Error message

38f8f3c

typo

385e8fd

Changed hash as putting it into a testfunction changed the funtion_id

5a0b125

dynamic funciton_id

e9bf342

import error

e559ab1

francescalb requested review from CasperWA and sygout October 18, 2023 11:37

jesper-friis requested review from jesper-friis and CasperWA and removed request for sygout and CasperWA October 18, 2023 14:46

jesper-friis reviewed Oct 18, 2023

View reviewed changes

tests/test_add_function.py Show resolved Hide resolved

jesper-friis approved these changes Oct 18, 2023

View reviewed changes

CasperWA requested changes Oct 19, 2023

View reviewed changes

francescalb and others added 2 commits October 19, 2023 10:07

Update .github/workflows/ci_tests.yml

1ce3ea8

Co-authored-by: Casper Welzel Andersen <[email protected]>

Update tripper/mappings/mappings.py

8f939f1

Co-authored-by: Casper Welzel Andersen <[email protected]>

francescalb added 3 commits October 19, 2023 10:33

update in requirements for core-testing with versions

d81f725

Moved some requirements according to reviewers desires

da314c6

unify naming of core-testing

6f05b11

francescalb requested a review from CasperWA October 19, 2023 08:50

CasperWA approved these changes Oct 19, 2023

View reviewed changes

Update tests/test_collection.py

3533c5b

Co-authored-by: Casper Welzel Andersen <[email protected]>

francescalb merged commit f9f43c0 into EMMC-ASBL:master Oct 19, 2023
14 checks passed

francescalb deleted the francescalb/testing_dependencies branch October 19, 2023 09:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Francescalb/testing dependencies #142

Francescalb/testing dependencies #142

francescalb commented Oct 17, 2023

codecov-commenter commented Oct 18, 2023 •

edited

Loading

jesper-friis left a comment •

edited

Loading

CasperWA left a comment

CasperWA Oct 19, 2023

francescalb Oct 19, 2023 •

edited

Loading

CasperWA Oct 19, 2023

francescalb Oct 19, 2023

CasperWA Oct 19, 2023

francescalb Oct 19, 2023

francescalb Oct 19, 2023

CasperWA left a comment

Francescalb/testing dependencies #142

Francescalb/testing dependencies #142

Conversation

francescalb commented Oct 17, 2023

Description:

Type of change:

Checklist for the reviewer:

codecov-commenter commented Oct 18, 2023 • edited Loading

Codecov Report

jesper-friis left a comment • edited Loading

Choose a reason for hiding this comment

CasperWA left a comment

Choose a reason for hiding this comment

CasperWA Oct 19, 2023

Choose a reason for hiding this comment

francescalb Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

CasperWA Oct 19, 2023

Choose a reason for hiding this comment

francescalb Oct 19, 2023

Choose a reason for hiding this comment

CasperWA Oct 19, 2023

Choose a reason for hiding this comment

francescalb Oct 19, 2023

Choose a reason for hiding this comment

francescalb Oct 19, 2023

Choose a reason for hiding this comment

CasperWA left a comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 18, 2023 •

edited

Loading

jesper-friis left a comment •

edited

Loading

francescalb Oct 19, 2023 •

edited

Loading