Sanitize MD and initiate addition of tests #43

yarikoptic · 2024-08-21T13:10:19Z

dependabot sends PRs with []() in the title eg

[gh-actions](deps): Bump actions/setup-python from 4 to 5

making it not legitimate markdown

[gh-actions: Bump actions/setup-python from 4 to 5]([gh-actions](deps): Bump actions/setup-python from 4 to 5 dandi/zarr_checksum#42) (253 days)

so with this change we just remove any of the [] symbols, making it a legit

gh-actions(deps): Bump actions/setup-python from 4 to 5 (253 days)

dependabot sends PRs with `[]()` in the title eg [gh-actions](deps): Bump actions/setup-python from 4 to 5 making it not legitimate markdown - [[gh-actions](deps): Bump actions/setup-python from 4 to 5](dandi/zarr_checksum#42) (253 days) so with this change we just remove any of the [] symbols, making it a legit - [gh-actions(deps): Bump actions/setup-python from 4 to 5](dandi/zarr_checksum#42) (253 days)

setup.cfg

jwodder · 2024-08-21T13:12:28Z

setup.cfg

+test =
+    mypy
+    pytest
+    pytest-cov


This isn't used by the test environment.

@yarikoptic pytest-cov is still unused.

Also, I would prefer it if, rather than defining a test extra, the test dependencies were instead listed in tox.ini.

why (what advantage) to list in a tool specific tox.ini making it harder to setup environment for any other tool/direct invocation of pytest? I would really prefer to concentrate dependencies in a single location.

I generally do not support any other tools than tox nor directly invoking pytest.

I typically invoke pytest directly, and testing within debian and most often conda packages is done directly via pytest. it is odd to require some additional utility whenever pytest framework is already sufficient.

jwodder · 2024-08-21T13:12:52Z

src/solidation/__main__.py

@@ -437,6 +437,11 @@ def ensure_aware(dt: datetime) -> datetime:
    return dt.replace(tzinfo=timezone.utc) if dt.tzinfo is None else dt


+def sanitize_md(s: str) -> str:
+    # Remove `[]` symbols to ensure correct markdown in the references
+    return re.sub(r'[\[\]]', '', s)


Wouldn't it be better to instead escape brackets, changing [ to \[?

dang, I didn't even try but it indeed seems to work:

[gh-actions](deps): Bump actions/setup-python from 4 to 5 (253 days)

so I guess we would also need to escape the \:

\[gh-actions](deps): Bump actions/setup-python from 4 to 5 (253 days)

ok, will do... now

jwodder · 2024-08-21T13:13:42Z

src/solidation/tests/test_main.py

@@ -0,0 +1,5 @@
+from ..__main__ import sanitize_md


Move this file to tests/test_main.py relative to the project root (i.e., outside of src/).

I really see little benefits and only inconveniences from such separation, but ok, will do, since you are the lead on this one.

done in 4978b8a which also required replicating listing of tests now for all other tox components which take care about source code

jwodder · 2024-08-21T13:13:59Z

tox.ini

+[testenv]
+extras = test
+commands =
+    python -m pytest -v {posargs} src


Change src to tests after moving the test file.

src/solidation/__main__.py

.github/workflows/test.yml

jwodder · 2024-08-21T14:05:15Z

.github/workflows/test.yml

@@ -0,0 +1,27 @@
+name: Unit Test


Suggested change

name: Unit Test

name: Test

Made it such since we also have Smoke Test. Makes it more explicit on what is the difference between the two.

jwodder · 2024-08-21T14:06:12Z

tests/__init__.py

This file is unnecessary and should be removed.

tox.ini

src/solidation/__main__.py

jwodder · 2024-08-21T14:12:55Z

setup.cfg

+test =
+    mypy
+    pytest
+    pytest-cov


@yarikoptic pytest-cov is still unused.

Also, I would prefer it if, rather than defining a test extra, the test dependencies were instead listed in tox.ini.

Co-authored-by: John T. Wodder II <[email protected]>

John: This has nothing to do with whether imports are sorted in the tests. src_paths = tests declares that any modules immediately within tests/ are to be treated as first-party packages for sorting purposes, but we shouldn't be importing anything from modules within tests/ anyway.

=== Do not change lines below === { "chain": [], "cmd": "pre-commit autoupdate", "exit": 0, "extra_inputs": [], "inputs": [], "outputs": [], "pwd": "." } ^^^ Do not change lines above ^^^

…test also removed unnecessary now dev installation of joblib # extras test vs tox.ini: Most recent debate on tox.ini vs extra_depends is at con/solidation#43 (comment) Although I appreciate the convenience of "tox" as an entry point for testing, I increasingly find no support for it to be "the" location for testing depends listing. Moreover I keep running into cases of needing to fish out test dependencies somewhere else (tox.ini, nox etc) which then can differ across projects, and also require me to adopt some avoidable runtime etc overhead from running those extra test shims whenever pytest is just good enough. My arguments for generally adopting an approach of specifying test dependencies in setup.{cfg,py} or other "generic" locations as optional are: - pytest, and its extentions are used (imported) inside the tests. I, in 99% of cases, do consider "tests" to be the part of the source code. I would not consider them part of the source code whenever there is an outside test battery which is developed/maintained independently of the source code. As such, I think that dependencies for tests, as part of the source code, should be listed alongside with dependencies for the build/installation/run time dependencies. Some distributions even do "import test" across entire source code distribution and thus tend to decide or to request exclusion from source distributions (IMHO the wrong step in most of the cases). Ref: dandi/dandi-schema#249 - It is unfortunate that there is no "standard" convention on how/where to specify such test requirements, so I think it is ok to adopt [test] as the general convention among our projects. - tox.ini looses NOTHING from using "extras". - tox.ini is the correct location to describe environments and dependncies for external to source code tools/modules, i.e those not imported explicitly anywhere in the source code. - with description of test dependencies alongside with the main dependencies would benefit downstream distribution (debian, conda, gentoo, etc) packagers since they do not need to fish around for what other dependencies to install/recommend/suggest for the package. - I do appreciate the fact that test dependencies alone are not sufficient to run the tests, but invocation of the pytest is standardized enough to just invoke it against the source code. (given dependencies are installed) That is e.g. how dh-python helper in Debian operates -- if pytest dependency announced, just run pytest automagically. # joblib: joblib dev listing was added in commit 6375816 Author: John T. Wodder II <[email protected]> Date: Fri Mar 19 08:42:47 2021 -0400 likely to use ❯ git describe --contains 457d2c8a926105fd156b1dfccfaae3e500d22451 1.1.0~13 ❯ git show 457d2c8a926105fd156b1dfccfaae3e500d22451 commit 457d2c8a926105fd156b1dfccfaae3e500d22451 Author: John T. Wodder II <[email protected]> Date: Thu Mar 18 06:07:06 2021 -0400 Use inspect.signature() instead of inspect.getfullargspec() (#1165) This fixes a bug with the ignore parameter when the cached function is decorated Co-authored-by: Loïc Estève <[email protected]> and we have already joblib ~= 1.1 so should be all set.

…test Most recent debate on tox.ini vs extra_depends is at con/solidation#43 (comment) Although I appreciate the convenience of "tox" as an entry point for testing, I increasingly find no support for it to be "the" location for testing depends listing. Moreover I keep running into cases of needing to fish out test dependencies somewhere else (tox.ini, nox etc) which then can differ across projects, and also require me to adopt some avoidable runtime etc overhead from running those extra test shims whenever pytest is just good enough. My arguments for generally adopting an approach of specifying test dependencies in setup.{cfg,py} or other "generic" locations as optional are: - pytest, and its extentions are used (imported) inside the tests. I, in 99% of cases, do consider "tests" to be the part of the source code. I would not consider them part of the source code whenever there is an outside test battery which is developed/maintained independently of the source code. As such, I think that dependencies for tests, as part of the source code, should be listed alongside with dependencies for the build/installation/run time dependencies. Some distributions even do "import test" across entire source code distribution and thus tend to decide or to request exclusion from source distributions (IMHO the wrong step in most of the cases). Ref: dandi/dandi-schema#249 - It is unfortunate that there is no "standard" convention on how/where to specify such test requirements, so I think it is ok to adopt [test] as the general convention among our projects. - tox.ini looses NOTHING from using "extras". - tox.ini is the correct location to describe environments and dependncies for external to source code tools/modules, i.e those not imported explicitly anywhere in the source code. - with description of test dependencies alongside with the main dependencies would benefit downstream distribution (debian, conda, gentoo, etc) packagers since they do not need to fish around for what other dependencies to install/recommend/suggest for the package. - I do appreciate the fact that test dependencies alone are not sufficient to run the tests, but invocation of the pytest is standardized enough to just invoke it against the source code. (given dependencies are installed) That is e.g. how dh-python helper in Debian operates -- if pytest dependency announced, just run pytest automagically.

yarikoptic requested a review from jwodder August 21, 2024 13:10

jwodder requested changes Aug 21, 2024

View reviewed changes

yarikoptic added 6 commits August 21, 2024 09:49

no mypy for test

9ace6e3

Escape, not remove, [] and \

b3ccc84

More of sanitize_md

8af3feb

Add workflow for running tests

65e4d0c

Move tests/ under the root of the project

4978b8a

Satisfy linters

190f483

yarikoptic requested a review from jwodder August 21, 2024 14:02

jwodder requested changes Aug 21, 2024

View reviewed changes

yarikoptic and others added 7 commits August 21, 2024 10:53

Fix name for CI step

9adbdb1

Co-authored-by: John T. Wodder II <[email protected]>

pre-commit fixups

15047f1

One more sanitize invocation I missed and John noted

6b57e0e

no pytest-cov

0d6d737

[DATALAD RUNCMD] pre-commit autoupdate

b56188a

=== Do not change lines below === { "chain": [], "cmd": "pre-commit autoupdate", "exit": 0, "extra_inputs": [], "inputs": [], "outputs": [], "pwd": "." } ^^^ Do not change lines above ^^^

rm tests/__init__.py since not needed according to @jwodder

ceedd36

yarikoptic mentioned this pull request Aug 23, 2024

centralize testing depends in setup.cfg extra depends con/fscacher#96

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sanitize MD and initiate addition of tests #43

Sanitize MD and initiate addition of tests #43

yarikoptic commented Aug 21, 2024

jwodder Aug 21, 2024

jwodder Aug 21, 2024

yarikoptic Aug 21, 2024

jwodder Aug 21, 2024

yarikoptic Aug 22, 2024

jwodder Aug 21, 2024

yarikoptic Aug 21, 2024

jwodder Aug 21, 2024

yarikoptic Aug 21, 2024

yarikoptic Aug 21, 2024

jwodder Aug 21, 2024

jwodder Aug 21, 2024

yarikoptic Aug 21, 2024

jwodder Aug 21, 2024

jwodder Aug 21, 2024

Sanitize MD and initiate addition of tests #43

Are you sure you want to change the base?

Sanitize MD and initiate addition of tests #43

Conversation

yarikoptic commented Aug 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment