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

Reorganize the tests into unit/functional/integration/external #1155

Merged
merged 37 commits into from
Nov 23, 2023
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
2493743
Move the test notebooks to tests/data/notebooks
mwouts Nov 6, 2023
555d0d7
Move the pre-commit tests to a subdir
mwouts Nov 6, 2023
eb0e77e
Split tests into unit/integration/functional
mwouts Nov 7, 2023
2ef289a
Split the test dependencies into test/integration/functional
mwouts Nov 9, 2023
32f8c92
Document test requirements with pytest markers
mwouts Nov 11, 2023
c1850be
Use packaging.version if available
mwouts Nov 11, 2023
e22b7be
Ignore a few more warnings
mwouts Nov 11, 2023
c442e3f
Move the codecov.yml configuration file to .github
mwouts Nov 11, 2023
3fcc046
Remove the world population notebook from the examples
mwouts Nov 11, 2023
4b3eec3
Move some tests to external
mwouts Nov 19, 2023
bf6b3e4
Test unit/functional/integration/external tests and their dependencies
mwouts Nov 19, 2023
0b9344d
Parametrize the tests by all the Python files in `src/jupytext`
mwouts Nov 19, 2023
f180532
Move tests with dependencies to integration/external
mwouts Nov 19, 2023
3d5b528
Move test on pre-commit mode
mwouts Nov 19, 2023
ab15185
Label as external a test on the rst2md option
mwouts Nov 19, 2023
a6ca72d
Fix import error
mwouts Nov 19, 2023
4e32850
Include tests in the Jupytext package
mwouts Nov 19, 2023
4406786
Use Python 3.x for the external tests
mwouts Nov 19, 2023
57ed3b4
Filter more of the expected warnings
mwouts Nov 19, 2023
7442c15
Install JupyterLab before testing the extension
mwouts Nov 19, 2023
8f29535
Remove isort_version
mwouts Nov 19, 2023
674b3aa
Remove matrix.external, kernel=true is the default
mwouts Nov 19, 2023
54189af
Install external dependencies too
mwouts Nov 19, 2023
096ff0d
Install JupyterLab before running UI tests
mwouts Nov 19, 2023
abc6676
Fix name of env
mwouts Nov 19, 2023
3b82bdf
Restore environment-ci.yml
mwouts Nov 20, 2023
595e154
Include step test_classification in the CI
mwouts Nov 21, 2023
1417ca7
Remove empty line
mwouts Nov 21, 2023
a2cd5c7
Turn the tests notebooks into fixtures
mwouts Nov 21, 2023
23fdd2d
packaging is a dependency
mwouts Nov 23, 2023
12b2201
Importing parse won't fail
mwouts Nov 23, 2023
6e10678
Try codecov flags
mwouts Nov 23, 2023
6ea8a80
Remove marker 'requires_jupytext'
mwouts Nov 23, 2023
12dbf54
Simplify the pytest command
mwouts Nov 23, 2023
4fb0dba
Move some tests to external
mwouts Nov 23, 2023
3c8608d
Rename test to avoid conflict
mwouts Nov 23, 2023
52c53b1
Fix the tests on Windows
mwouts Nov 23, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
7 changes: 3 additions & 4 deletions codecov.yml → .github/codecov.yml
Original file line number Diff line number Diff line change
@@ -1,21 +1,20 @@
codecov:
notify:
after_n_builds: 11
notify:
after_n_builds: 11

comment:
after_n_builds: 11

coverage:
status:
project:
default: false # disable the default status that measures entire project
tests:
paths:
- "tests/"
target: 100%
source:
paths:
- "jupytext/"
- "src/jupytext/"
target: 97%
threshold: 0.002
patch:
Expand Down
2 changes: 1 addition & 1 deletion .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
exclude: >
(?x)^(
demo/.*|
tests/notebooks/.*|
tests/data/notebooks/.*|
)$
repos:

Expand Down
4 changes: 2 additions & 2 deletions docs/contributing.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,5 @@ You want to submit an enhancement on Jupytext? Unless this is a small change, we
A pull request for which you do not need to contact us in advance is the addition of a new language to Jupytext. In principle that should be easy - you would only have to:
- document the language extension and comment by adding one line to `_SCRIPT_EXTENSIONS` in `jupytext/languages.py`.
- add the language to `docs/languages.md`
- contribute a sample notebook in `tests/notebooks/ipynb_[language]`.
- run the tests suite (create a [development environment](developing.md), then execute `pytest` locally). The tests will generate various text representations corresponding to your notebook under `tests/notebooks/mirror/`. Please verify that these files are valid scripts, and include them in your PR.
- contribute a sample notebook in `tests/data/notebooks/inputs/ipynb_[language]`.
- run the tests suite (create a [development environment](developing.md), then execute `pytest` locally). The tests will generate various text representations corresponding to your notebook under `tests/data/notebooks/outputs/`. Please verify that these files are valid scripts, and include them in your PR.
4 changes: 2 additions & 2 deletions docs/using-pre-commit.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,5 +62,5 @@ repos:
language_version: python3
```

Tested examples of how to use the pre-commit hook are available in our [tests](https://github.com/mwouts/jupytext/tree/main/tests) -
see for instance [test_pre_commit_1_sync_with_config.py](https://github.com/mwouts/jupytext/blob/main/tests/test_pre_commit_1_sync_with_config.py).
Tested examples of how to use the pre-commit hook are available in our [tests](https://github.com/mwouts/jupytext/tree/main/tests/functional/pre-commit) -
see for instance [test_pre_commit_1_sync_with_config.py](https://github.com/mwouts/jupytext/blob/main/tests/functional/pre-commit/test_pre_commit_1_sync_with_config.py).
66 changes: 48 additions & 18 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -46,27 +46,34 @@ Documentation = "https://jupytext.readthedocs.io"

[project.optional-dependencies]
# Test related dependencies
# TODO: Split them into unit, integration and functional tests groups
test = [
"autopep8",
"black",
"isort",
"ruff",
"flake8",
"pytest",
"pytest-randomly",
"gitpython",
"jupyterlab",
"notebook",
"nbconvert",
# jupyter-fs==0.4.0 is async, which is not supported by Jupytext ATM
"jupyter-fs<0.4.0",
"ipykernel",
"pre-commit",
"pytest",
"pytest-randomly"
]
test-integration = [
"jupytext[test]",
"jupyter-server",
"nbconvert"
]
test-functional = [
"jupytext[test-integration]",
# jupytext --pipe and --check
"autopep8",
"black",
"isort",
"flake8",
# jupytext --execute
"ipykernel",
# Pre-commit tests
"gitpython",
"pre-commit",
# Interaction with other contents managers
# jupyter-fs==0.4.0 is async, which is not supported by Jupytext ATM
"jupyter-fs<0.4.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer thse in an integration test, mostly because these are optional dependencies, and they primarily test how jupytext interacts with other tools. Indeed the tests are better classified as functional tests, and I see many projects do so, so it is more of a subjective classification.

Why I would prefer this as integration is because w.r.t. packaging, unit and functional tests are good to include as a whole, while integration tests are a bit hit-and-miss (mostly because most projects use mocking for these, and the package versions can differ drastically with the one tested upstream).

We could also move these to test-external as the optional-dependency (those that are optional) and keep the unit/integration/functional` categorization like this.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks for the guidance! Actually I have moved all the tests that involve a non-core dependency to tests/external.

All the tests in unit, functional and integration pass, with very little dependencies. I think all these tests should always pass.

I have also included the tests in the package (but I was not sure whether tests/external should be included too?)

Copy link
Contributor

Choose a reason for hiding this comment

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

About packaging, yes it is better to be safe and include everything. Also, sdist only picks up on git sources (non-git sources are added as artifacts), and the only other one in the list is demo which is good to include as well

Copy link
Contributor

@LecrisUT LecrisUT Nov 20, 2023

Choose a reason for hiding this comment

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

About the tests 👍 to the organization. Just a comment, probably you are aware of the practice of mirroring the source files with the test files, but that is only meaningful in unit-tests, which you seem to be following, so 👍. Also, a coding practice tip, it is good to split the codecov flags into unit-test and the rest.

There is one confusing part, why areall of the tests as python packages? Pytest should automatically resolve them from the test_* naming scheme.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks @LecrisUT !

Can you point me to an exemple re the differentiated coverage ?

Re the inits in tests, indeed I am planning to remove them in this PR. Right now I need them because they import from utils.py but I do plan to use fixtures instead of list_notebooks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

Nice! Thanks for sharing! I'll try this asap.

]
# Coverage requirements
test-cov = [
"jupytext[test]",
"jupytext[test-functional]",
"pytest-cov>=2.6.1",
]
dev = [
Expand Down Expand Up @@ -163,7 +170,7 @@ ignore = ["W002"]
[tool.ruff]
line-length = 127
exclude = [
"tests/notebooks/*",
"tests/data/notebooks/*",
]
# Seems like W503 is not implemented in ruff
# ref: https://github.com/astral-sh/ruff/issues/4125
Expand All @@ -172,9 +179,32 @@ ignore = [
]

[tool.pytest.ini_options]
markers = [
"requires_jupytext",
"requires_black",
"requires_isort",
"requires_flake8",
"requires_autopep8",
"requires_nbconvert",
"requires_myst",
"requires_no_myst",
"requires_quarto",
"requires_pandoc",
"requires_no_pandoc",
"requires_sphinx_gallery",
"requires_user_kernel_python3",
"requires_ir_kernel",
mwouts marked this conversation as resolved.
Show resolved Hide resolved
"skip_on_windows",
"pre_commit",
]
filterwarnings = [
# Uncomment this "error" to turn all unfiltered warnings into errors
# "error",
# Jupyter notebook
"ignore:Support for bleach <5 will be removed in a future version of nbconvert:DeprecationWarning",
# jupyterfs
"ignore:run_pre_save_hook is deprecated, use run_pre_save_hooks instead:DeprecationWarning",
"ignore:Deprecated call to `pkg_resources.declare_namespace:DeprecationWarning",
mwouts marked this conversation as resolved.
Show resolved Hide resolved
# use single quote to denote raw strings in toml
# (10 warnings)
'ignore:Passing unrecognized arguments to super\(KernelSpec\).__init__:DeprecationWarning',
Expand Down
7 changes: 3 additions & 4 deletions src/jupytext/parse_version.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
def parse_version(version, custom_error=ImportError):
try:
from pkg_resources import parse_version as parse
from packaging.version import parse
mwouts marked this conversation as resolved.
Show resolved Hide resolved
except ImportError:
try:
from packaging.version import parse
from pkg_resources import parse_version as parse
except ImportError:
raise custom_error("Please install either pkg_resources or packaging")
raise custom_error("Please install either packaging or pkg_resources")

print(version)
return parse(version)
84 changes: 82 additions & 2 deletions tests/conftest.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import sys
import unittest.mock as mock
from pathlib import Path

import pytest
from git import Repo
from jupyter_client.kernelspec import find_kernel_specs, get_kernel_spec
from nbformat.v4 import nbbase
from nbformat.v4.nbbase import (
new_code_cell,
Expand All @@ -12,15 +14,28 @@
)

import jupytext
from jupytext.cell_reader import rst2md
from jupytext.cli import system
from jupytext.myst import is_myst_available
from jupytext.pandoc import is_pandoc_available
from jupytext.quarto import is_quarto_available

from .utils import formats_with_support_for_cell_metadata
from .utils import formats_with_support_for_cell_metadata, tool_version

# Pytest's tmpdir is in /tmp (at least for me), so this helps avoiding interferences between
# Pytest's tmpdir is in /tmp (at least for me), so this helps to avoid interferences between
# global configuration on HOME and the test collection
jupytext.config.JUPYTEXT_CEILING_DIRECTORIES = ["/tmp/"]


def isort_version():
try:
import isort

return isort.__version__
except ImportError:
return None
mwouts marked this conversation as resolved.
Show resolved Hide resolved


@pytest.fixture
def no_jupytext_version_number():
with mock.patch("jupytext.header.INSERT_AND_CHECK_VERSION_NUMBER", False):
Expand Down Expand Up @@ -107,6 +122,71 @@ def fmt_with_cell_metadata(request):
return request.param


def pytest_runtest_setup(item):
for mark in item.iter_markers():
for tool in [
"jupytext",
"black",
"isort",
"flake8",
"autopep8",
"jupyter nbconvert",
]:
if mark.name == f"requires_{tool.replace(' ', '_')}":
if not tool_version(tool):
pytest.skip(f"{tool} is not installed")
mwouts marked this conversation as resolved.
Show resolved Hide resolved
if mark.name == "requires_sphinx_gallery":
if not rst2md:
pytest.skip("sphinx_gallery is not available")
if mark.name == "requires_pandoc":
# The mirror files changed slightly when Pandoc 2.11 was introduced
# https://github.com/mwouts/jupytext/commit/c07d919702999056ce47f92b74f63a15c8361c5d
# The mirror files changed again when Pandoc 2.16 was introduced
# https://github.com/mwouts/jupytext/pull/919/commits/1fa1451ecdaa6ad8d803bcb6fb0c0cf09e5371bf
if not is_pandoc_available(min_version="2.16.2", max_version="2.16.2"):
pytest.skip("pandoc==2.16.2 is not available")
if mark.name == "requires_quarto":
if not is_quarto_available(min_version="0.2.0"):
pytest.skip("quarto>=0.2 is not available")
if mark.name == "requires_no_pandoc":
if is_pandoc_available():
pytest.skip("Pandoc is installed")
if mark.name == "requires_ir_kernel":
if not any(
get_kernel_spec(name).language == "R" for name in find_kernel_specs()
):
pytest.skip("irkernel is not installed")
if mark.name == "requires_user_kernel_python3":
if "python_kernel" not in find_kernel_specs():
pytest.skip(
"Please run 'python -m ipykernel install --name python_kernel --user'"
)
if mark.name == "requires_myst":
if not is_myst_available():
pytest.skip("myst_parser not found")
if mark.name == "requires_no_myst":
if is_myst_available():
pytest.skip("myst is available")
if mark.name == "skip_on_windows":
if sys.platform.startswith("win"):
pytest.skip("Issue 489")
if mark.name == "pre_commit":
if sys.platform.startswith("win"):
pytest.skip(
"OSError: [WinError 193] %1 is not a valid Win32 application"
)
if not (Path(__file__).parent.parent / ".git").is_dir():
pytest.skip("Jupytext folder is not a git repository #814")


def pytest_collection_modifyitems(config, items):
for item in items:
if (
config.rootdir / "tests" / "functional" / "pre_commit"
) in item.path.parents:
item.add_marker(pytest.mark.pre_commit)


"""To make sure that cell ids are distinct we use a global counter.
This solves https://github.com/mwouts/jupytext/issues/747"""
global_cell_count = 0
Expand Down
Loading
Loading