Replace `tests/unit/test_doc.py` with a pre-commit hook. #56727

s0undt3ch · 2020-04-20T13:09:23Z

What does this PR do?

Replace tests/unit/test_doc.py with a pre-commit hook.

We don't need to test docs on all platforms. In turn, we make the check
a pre-commit hook which seems more suited.

Check that every module has associated documentation file
Check that every doc page has associated module file
Check that every module is listed in the index file
Check if the module name stomps on a virtual doc page name
Check that a module index is sorted
Check for duplicates inside of a module index
Check for stray module docs in index

Previous Behavior

We relied on a test case when ran on all platforms.

New Behavior

It's now a pre-commit hook which is check when committing code locally and also on PRs and branch builds, once.

bryceml · 2020-04-20T21:13:36Z

.pre-commit-config.yaml

+      - id: nox-py2
+        alias: check-docks
+        name: Check Docs
+        files: ^salt/.*\.py$


shouldn't we check if doc/ref changes as well?

bryceml · 2020-04-20T21:13:53Z

.pre-commit-config.yaml

+    rev: master
+    hooks:
+      - id: nox-py2
+        alias: check-docks


docks seems like a misspelling to me

max-arnold · 2020-04-21T02:15:46Z

It looks like not all of the recently merged doc tests were ported to invoke tasks. Is that intentional?

mchugh19 · 2020-04-21T05:02:09Z

As @max-arnold pointed out, this appears to only be checking for that any module has an associated ref file, and that :doc: isn't inline. While it is cool to migrate these checks to pre-commit, it doesn't capture the same functionality of #56727.

Every one of the checks from that PR caught a problem. Since those checks are not being migrated, was it determined that was a single one time activity and that they aren't needed, or is there more to come before doc_test is removed?

doc_test functionality includes:

Check that every module has associated documentation file ✅
Check that every doc page has associated module file
Check that every module is listed in the index file
Check if the module name stomps on a virtual doc page name
Check that a module index is sorted
Check for duplicates inside of a module index
Check for stray module docs in index

s0undt3ch · 2020-04-21T06:49:55Z

Thank You for your input @max-arnold and @mchugh19

The reduced functionality was not intentional, it was an oversight, I guess I was too focused getting something working at the time and didn't look carefully at the issues you addressed on your PR.

This should not be merged until all issues you solved have been addressed.

We don't need to test docs on all platforms. In turn, we make the check a pre-commit hook which seems more suited.

s0undt3ch · 2020-04-23T19:02:44Z

I think I addressed all concerns. Can you please review @max-arnold and @mchugh19?

mchugh19 · 2020-04-24T05:24:45Z

Nice! Looks good to me

max-arnold · 2020-04-24T06:31:26Z

@s0undt3ch I'm probably doing something wrong, but I get this:

% git diff --cached
diff --git a/doc/ref/auth/all/index.rst b/doc/ref/auth/all/index.rst
index cad05ba139..d421efbbe7 100644
--- a/doc/ref/auth/all/index.rst
+++ b/doc/ref/auth/all/index.rst
@@ -12,7 +12,6 @@ auth modules

     auto
     django
-    file
     keystone
     ldap
     mysql

% pre-commit run

[INFO] Initializing environment for https://github.com/saltstack/pip-tools-compile-impersonate.
[INFO] Initializing environment for https://github.com/timothycrosley/isort.
[INFO] Initializing environment for https://github.com/timothycrosley/isort:toml.
[INFO] Initializing environment for https://github.com/psf/black.
[INFO] Initializing environment for https://github.com/saltstack/salt-nox-pre-commit.
[INFO] Installing environment for https://github.com/saltstack/pip-tools-compile-impersonate.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/timothycrosley/isort.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/psf/black.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/saltstack/salt-nox-pre-commit.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
Linux Py3.5 ZeroMQ Requirements......................(no files to check)Skipped
Darwin Py3.5 ZeroMQ Requirements.....................(no files to check)Skipped
Windows Py3.5 ZeroMQ Requirements....................(no files to check)Skipped
Cloud Py3.5 Requirements.............................(no files to check)Skipped
Docs Py3.5 Requirements..............................(no files to check)Skipped
Linux Py3.5 Crypto Requirements......................(no files to check)Skipped
Darwin Py3.5 Crypto Requirements.....................(no files to check)Skipped
Windows Py3.5 Crypto Requirements....................(no files to check)Skipped
Lint Py3.5 Requirements..............................(no files to check)Skipped
Linux Py3.6 ZeroMQ Requirements......................(no files to check)Skipped
Darwin Py3.6 ZeroMQ Requirements.....................(no files to check)Skipped
Windows Py3.6 ZeroMQ Requirements....................(no files to check)Skipped
Cloud Py3.6 Requirements.............................(no files to check)Skipped
Docs Py3.6 Requirements..............................(no files to check)Skipped
Linux Py3.6 Crypto Requirements......................(no files to check)Skipped
Darwin Py3.6 Crypto Requirements.....................(no files to check)Skipped
Windows Py3.6 Crypto Requirements....................(no files to check)Skipped
Lint Py3.6 Requirements..............................(no files to check)Skipped
Linux Py3.7 ZeroMQ Requirements......................(no files to check)Skipped
Darwin Py3.7 ZeroMQ Requirements.....................(no files to check)Skipped
Windows Py3.7 ZeroMQ Requirements....................(no files to check)Skipped
Cloud Py3.7 Requirements.............................(no files to check)Skipped
Docs Py3.7 Requirements..............................(no files to check)Skipped
Linux Py3.7 Crypto Requirements......................(no files to check)Skipped
Darwin Py3.7 Crypto Requirements.....................(no files to check)Skipped
Windows Py3.7 Crypto Requirements....................(no files to check)Skipped
Lint Py3.7 Requirements..............................(no files to check)Skipped
Linux Py3.8 ZeroMQ Requirements......................(no files to check)Skipped
Darwin Py3.8 ZeroMQ Requirements.....................(no files to check)Skipped
Cloud Py3.8 Requirements.............................(no files to check)Skipped
Docs Py3.8 Requirements..............................(no files to check)Skipped
Linux Py3.8 Crypto Requirements......................(no files to check)Skipped
Darwin Py3.8 Crypto Requirements.....................(no files to check)Skipped
Lint Py3.8 Requirements..............................(no files to check)Skipped
Linux Py3.9 ZeroMQ Requirements......................(no files to check)Skipped
Darwin Py3.9 ZeroMQ Requirements.....................(no files to check)Skipped
Cloud Py3.9 Requirements.............................(no files to check)Skipped
Docs Py3.9 Requirements..............................(no files to check)Skipped
Linux Py3.9 Crypto Requirements......................(no files to check)Skipped
Darwin Py3.9 Crypto Requirements.....................(no files to check)Skipped
Lint Py3.9 Requirements..............................(no files to check)Skipped
Linux Py3.5 Invoke Requirements......................(no files to check)Skipped
Linux Py3.6 Invoke Requirements......................(no files to check)Skipped
Linux Py3.7 Invoke Requirements......................(no files to check)Skipped
Linux Py3.8 Invoke Requirements......................(no files to check)Skipped
Linux Py3.9 Invoke Requirements......................(no files to check)Skipped
isort................................................(no files to check)Skipped
black................................................(no files to check)Skipped
Lint Salt............................................(no files to check)Skipped
Lint Tests...........................................(no files to check)Skipped
Check Docs...............................................................Failed
- hook id: nox-py2
- exit code: 1

Running session invoke-pre-commit
Creating virtualenv using python3 in .nox/invoke-pre-commit
pip install --progress-bar=off -r requirements/static/invoke.in --constraint requirements/static/py3.7/invoke.txt
inv docs.check --files=doc/ref/auth/all/index.rst
Checking inline :doc: markup
Checking python module stubs
Checking virtual modules
Traceback (most recent call last):
  File "/Users/user/.cache/pre-commit/repozcmrljxh/py_env-python3/bin/inv", line 10, in <module>
    sys.exit(program.run())
  File "/Users/user/.cache/pre-commit/repozcmrljxh/py_env-python3/lib/python3.7/site-packages/invoke/program.py", line 384, in run
    self.execute()
  File "/Users/user/.cache/pre-commit/repozcmrljxh/py_env-python3/lib/python3.7/site-packages/invoke/program.py", line 566, in execute
    executor.execute(*self.tasks)
  File "/Users/user/.cache/pre-commit/repozcmrljxh/py_env-python3/lib/python3.7/site-packages/invoke/executor.py", line 129, in execute
    result = call.task(*args, **call.kwargs)
  File "/Users/user/.cache/pre-commit/repozcmrljxh/py_env-python3/lib/python3.7/site-packages/invoke/tasks.py", line 127, in __call__
    result = self.body(*args, **kwargs)
  File "/Users/user/repos/saltstack-repo/tasks/docs.py", line 444, in check
    check_virtual(ctx, files)
  File "/Users/user/.cache/pre-commit/repozcmrljxh/py_env-python3/lib/python3.7/site-packages/invoke/tasks.py", line 127, in __call__
    result = self.body(*args, **kwargs)
  File "/Users/user/repos/saltstack-repo/tasks/docs.py", line 230, in check_virtual
    files = build_docs_paths(files)
  File "/Users/user/repos/saltstack-repo/tasks/docs.py", line 159, in build_docs_paths
    return build_file_list(files, ".rst")
  File "/Users/user/repos/saltstack-repo/tasks/docs.py", line 140, in build_file_list
    _files = [path.relative_to(CODE_DIR) for path in _files]
  File "/Users/user/repos/saltstack-repo/tasks/docs.py", line 140, in <listcomp>
    _files = [path.relative_to(CODE_DIR) for path in _files]
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/pathlib.py", line 895, in relative_to
    .format(str(self), str(formatted)))
ValueError: 'doc/ref/auth/all/index.rst' does not start with '/Users/user/repos/saltstack-repo'
Command inv docs.check --files=doc/ref/auth/all/index.rst failed with exit code 1
Session invoke-pre-commit failed.

s0undt3ch · 2020-04-24T06:49:40Z

❯ git diff --cached 
diff --git a/doc/ref/auth/all/index.rst b/doc/ref/auth/all/index.rst
index cad05ba139..d421efbbe7 100644
--- a/doc/ref/auth/all/index.rst
+++ b/doc/ref/auth/all/index.rst
@@ -12,7 +12,6 @@ auth modules
 
     auto
     django
-    file
     keystone
     ldap
     mysql

❯ nox -e invoke -- docs.check
nox > Running session invoke
nox > Re-using existing virtualenv at .nox/invoke.
nox > pip install --progress-bar=off -r requirements/static/invoke.in --constraint requirements/static/py3.5/invoke.txt
nox > inv docs.check
Checking inline :doc: markup
Checking python module stubs
Checking virtual modules
Checking doc module indexes
The module index at doc/ref/auth/all/index.rst is missing the following modules: file
nox > Command inv docs.check failed with exit code 1
nox > Session invoke failed.

However

❯ pre-commit run check-docs
Check Docs...............................................................Failed
- hook id: nox-py2
- exit code: 1

Running session invoke-pre-commit
Re-using existing virtualenv at .nox/invoke-pre-commit.
pip install --progress-bar=off -r requirements/static/invoke.in --constraint requirements/static/py3.6/invoke.txt
inv docs.check --files=doc/ref/auth/all/index.rst
Checking inline :doc: markup
Checking python module stubs
Checking virtual modules
Traceback (most recent call last):
  File "/home/vampas/.cache/pre-commit/reporZk5DS/py_env-python3/bin/inv", line 8, in <module>
    sys.exit(program.run())
  File "/home/vampas/.cache/pre-commit/reporZk5DS/py_env-python3/lib/python3.6/site-packages/invoke/program.py", line 384, in run
    self.execute()
  File "/home/vampas/.cache/pre-commit/reporZk5DS/py_env-python3/lib/python3.6/site-packages/invoke/program.py", line 566, in execute
    executor.execute(*self.tasks)
  File "/home/vampas/.cache/pre-commit/reporZk5DS/py_env-python3/lib/python3.6/site-packages/invoke/executor.py", line 129, in execute
    result = call.task(*args, **call.kwargs)
  File "/home/vampas/.cache/pre-commit/reporZk5DS/py_env-python3/lib/python3.6/site-packages/invoke/tasks.py", line 127, in __call__
    result = self.body(*args, **kwargs)
  File "/home/vampas/projects/SaltStack/salt/features/doc-pre-commit/tasks/docs.py", line 444, in check
    check_virtual(ctx, files)
  File "/home/vampas/.cache/pre-commit/reporZk5DS/py_env-python3/lib/python3.6/site-packages/invoke/tasks.py", line 127, in __call__
    result = self.body(*args, **kwargs)
  File "/home/vampas/projects/SaltStack/salt/features/doc-pre-commit/tasks/docs.py", line 230, in check_virtual
    files = build_docs_paths(files)
  File "/home/vampas/projects/SaltStack/salt/features/doc-pre-commit/tasks/docs.py", line 159, in build_docs_paths
    return build_file_list(files, ".rst")
  File "/home/vampas/projects/SaltStack/salt/features/doc-pre-commit/tasks/docs.py", line 140, in build_file_list
    _files = [path.relative_to(CODE_DIR) for path in _files]
  File "/home/vampas/projects/SaltStack/salt/features/doc-pre-commit/tasks/docs.py", line 140, in <listcomp>
    _files = [path.relative_to(CODE_DIR) for path in _files]
  File "/home/vampas/.dotfiles/.ext/pyenv/versions/3.6.8/lib/python3.6/pathlib.py", line 874, in relative_to
    .format(str(self), str(formatted)))
ValueError: 'doc/ref/auth/all/index.rst' does not start with '/home/vampas/projects/SaltStack/salt/features/doc-pre-commit'
Command inv docs.check --files=doc/ref/auth/all/index.rst failed with exit code 1
Session invoke-pre-commit failed.

s0undt3ch · 2020-04-24T06:50:07Z

Working on a fix

s0undt3ch · 2020-04-24T06:56:18Z

The fix is in #56875

s0undt3ch · 2020-04-24T06:56:29Z

Thank You @max-arnold !

max-arnold · 2020-04-24T11:06:11Z

I briefly tried other types of checks. The only problem I found is that pre-commit command doesn't check deleted doc files (or I'm using it in a wrong way):

(salt-tests) ~/repos/saltstack-repo % git rm doc/ref/beacons/all/salt.beacons.adb.rst
rm 'doc/ref/beacons/all/salt.beacons.adb.rst'

(salt-tests) ~/repos/saltstack-repo % pre-commit run check-docs
Check Docs...........................................(no files to check)Skipped

It works when called via nox:

(salt-tests) ~/repos/saltstack-repo % nox -e invoke -- docs.check-stubs
nox > Running session invoke
nox > Re-using existing virtual environment at .nox/invoke.
nox > pip install --progress-bar=off -r requirements/static/invoke.in --constraint requirements/static/py3.7/invoke.txt
nox > inv docs.check-stubs
The module at salt/beacons/adb.py does not have a sphinx stub at doc/ref/beacons/all/salt.beacons.adb.rst
nox > Command inv docs.check-stubs failed with exit code 1
nox > Session invoke failed.

s0undt3ch force-pushed the features/doc-pre-commit branch 2 times, most recently from 708f2c0 to b945768 Compare April 20, 2020 14:21

s0undt3ch marked this pull request as ready for review April 20, 2020 20:53

s0undt3ch requested a review from a team as a code owner April 20, 2020 20:53

ghost requested review from cmcmarrow and removed request for a team April 20, 2020 20:53

s0undt3ch force-pushed the features/doc-pre-commit branch from b945768 to 4162d69 Compare April 20, 2020 20:57

bryceml reviewed Apr 20, 2020

View reviewed changes

.pre-commit-config.yaml Outdated

rev: master

hooks:

- id: nox-py2

alias: check-docks

Copy link

Contributor

bryceml Apr 20, 2020

Choose a reason for hiding this comment

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

docks seems like a misspelling to me

Lay the ground for invoke usage

a5b9961

s0undt3ch force-pushed the features/doc-pre-commit branch from 4162d69 to 0a701df Compare April 23, 2020 17:42

s0undt3ch added 2 commits April 23, 2020 20:01

Replace tests/unit/test_doc.py with a pre-commit hook.

6a47031

We don't need to test docs on all platforms. In turn, we make the check a pre-commit hook which seems more suited.

Add loader checks although not currently enforcing them

f6c2355

s0undt3ch force-pushed the features/doc-pre-commit branch from 0a701df to f6c2355 Compare April 23, 2020 19:01

dwoz approved these changes Apr 24, 2020

View reviewed changes

dwoz merged commit 096dc75 into saltstack:master Apr 24, 2020

s0undt3ch mentioned this pull request Apr 24, 2020

Resolve paths passed directly with --files #56875

Merged

sagetherage added the ZRelease-Sodium retired label label May 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `tests/unit/test_doc.py` with a pre-commit hook. #56727

Replace `tests/unit/test_doc.py` with a pre-commit hook. #56727

s0undt3ch commented Apr 20, 2020 •

edited

Loading

bryceml Apr 20, 2020

bryceml Apr 20, 2020

max-arnold commented Apr 21, 2020

mchugh19 commented Apr 21, 2020

s0undt3ch commented Apr 21, 2020

s0undt3ch commented Apr 23, 2020

mchugh19 commented Apr 24, 2020

max-arnold commented Apr 24, 2020 •

edited

Loading

s0undt3ch commented Apr 24, 2020

s0undt3ch commented Apr 24, 2020

s0undt3ch commented Apr 24, 2020

s0undt3ch commented Apr 24, 2020

max-arnold commented Apr 24, 2020

Replace tests/unit/test_doc.py with a pre-commit hook. #56727

Replace tests/unit/test_doc.py with a pre-commit hook. #56727

Conversation

s0undt3ch commented Apr 20, 2020 • edited Loading

What does this PR do?

Previous Behavior

New Behavior

bryceml Apr 20, 2020

Choose a reason for hiding this comment

bryceml Apr 20, 2020

Choose a reason for hiding this comment

max-arnold commented Apr 21, 2020

mchugh19 commented Apr 21, 2020

s0undt3ch commented Apr 21, 2020

s0undt3ch commented Apr 23, 2020

mchugh19 commented Apr 24, 2020

max-arnold commented Apr 24, 2020 • edited Loading

s0undt3ch commented Apr 24, 2020

s0undt3ch commented Apr 24, 2020

s0undt3ch commented Apr 24, 2020

s0undt3ch commented Apr 24, 2020

max-arnold commented Apr 24, 2020

Replace `tests/unit/test_doc.py` with a pre-commit hook. #56727

Replace `tests/unit/test_doc.py` with a pre-commit hook. #56727

s0undt3ch commented Apr 20, 2020 •

edited

Loading

max-arnold commented Apr 24, 2020 •

edited

Loading