Skip to content

Implement git submodules for specs #182

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

Open
wants to merge 14 commits into
base: add-github-workflow
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,9 @@ lib/PyLD.egg-info
profiler
tests/test_caching.py
tests/data/test_caching.json

# Local Python version with `pyenv`
.python-version

# PyCharm & other JetBrains IDEs
.idea
11 changes: 11 additions & 0 deletions .gitmodules
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
[submodule "specifications/json-ld-api"]
Copy link
Member

Choose a reason for hiding this comment

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

These are the specs, but since their sole purpose here is for testing, could this be named test-suites/? That's what the scripts in jsonld.js use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reviewing!

I can rename those. A few things concern me a bit though:

  • Each repository we're checking out contains not just tests but the whole specification, which might be confusing and violate the principle of least surprise;
  • We will likely add more tests to the tests directory which will not be using the specifications, and the existence of both tests and test-suites will be confusing;
  • At some point, we might want to use the specifications for other purposes than tests, for instance, to aid documentation generation, or something.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer the specifications/ folder name, as I do hope/expect we'll get more unit tests added to tests/ at some point, and test-suites/ would certainly be confusing.

path = specifications/json-ld-api
url = [email protected]:w3c/json-ld-api.git

[submodule "specifications/json-ld-framing"]
path = specifications/json-ld-framing
url = [email protected]:w3c/json-ld-framing.git

[submodule "specifications/rdf-dataset-canonicalization"]
path = specifications/rdf-dataset-canonicalization
url = [email protected]:w3c-ccg/rdf-dataset-canonicalization.git
5 changes: 5 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
upgrade-submodules:
git submodule update --remote --init --recursive
Copy link
Member

Choose a reason for hiding this comment

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

How do you remove all the checked out dirs without leaving git thinking there are unstaged changes? Put another way, how do you undo this command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you want to undo the version upgrade of the submodules, you can do:

git reset --hard

If you've modified the submodules locally, after having them upgraded (for debugging purposes perhaps) then you do:

git submodule foreach git reset --hard


test:
python tests/runtests.py
23 changes: 17 additions & 6 deletions README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -190,16 +190,27 @@ Tests
This library includes a sample testing utility which may be used to verify
that changes to the processor maintain the correct output.

To run the sample tests you will need to get the test suite files by cloning
the ``json-ld-api``, ``json-ld-framing``, and ``normalization`` repositories
hosted on GitHub:
To run the sample tests you will need the test suite files provided in the
``json-ld-api``, ``json-ld-framing``, and ``rdf-dataset-canonicalization``
repositories hosted
on GitHub:

- https://github.com/w3c/json-ld-api
- https://github.com/w3c/json-ld-framing
- https://github.com/json-ld/normalization
- https://github.com/w3c-ccg/rdf-dataset-canonicalization

If the suites repositories are available as sibling directories of the PyLD
source directory, then all the tests can be run with the following:
They are included beneath ``specifications`` directory of this repository as
Git submodules. By default, ``git clone`` does
not retrieve submodules; to download them, please issue the following command:

.. code-block:: bash

make upgrade-submodules

Issue the same command to update each submodule to the latest available commit.

If the suites repositories are available then all the tests can be run with
the following:

.. code-block:: bash

Expand Down
1 change: 1 addition & 0 deletions specifications/json-ld-api
Submodule json-ld-api added at 229c82
1 change: 1 addition & 0 deletions specifications/json-ld-framing
Submodule json-ld-framing added at f13920
1 change: 1 addition & 0 deletions specifications/rdf-dataset-canonicalization
28 changes: 15 additions & 13 deletions tests/runtests.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
import unittest
import re
from argparse import ArgumentParser
from pathlib import Path
from typing import List
from unittest import TextTestResult

sys.path.insert(0, os.path.join(os.path.dirname(__file__), '..', 'lib'))
Expand All @@ -32,9 +34,20 @@
LOCAL_BASES = [
'https://w3c.github.io/json-ld-api/tests',
'https://w3c.github.io/json-ld-framing/tests',
'https://github.com/json-ld/normalization/tests'
'https://github.com/w3c-ccg/rdf-dataset-canonicalization/tree/main/tests',
]


def default_test_targets() -> List[Path]:
Copy link
Member

Choose a reason for hiding this comment

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

How about changing this so it works something like the jsonld.js test runner and checks for the local test suite dir, then for the sibling dir if not found. That allows for alternate workflows.

Copy link
Member

Choose a reason for hiding this comment

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

It didn't consider missing test suites an error before. I'm not sure what should happen for that. Never seemed a problem in practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you believe there is a use case for it? Probably the main goal of turning specifications into submodules is to automate the connection to them, standardizing it across the developers' clones of pyld repo.

Having the specifications cloned into the root directory of the repo, for instance, will require .gitignore rules (or it's way too easy to commit them, which is likely not what one wants).

We can later add an error message to this function. If the default test locations are used, and if tests are missing, then probably something is wrong. For instance, the developer forgot to check out the submodules. Their tests will be incorrectly green locally, and then, after a push, failing CI will pose an unpleasant surprise.

"""Default test directories from specifications."""
specifications = Path(__file__).parent.parent / 'specifications'
return [
specifications / 'json-ld-api/tests',
specifications / 'json-ld-framing/tests',
specifications / 'rdf-dataset-canonicalization/tests',
]


class TestRunner(unittest.TextTestRunner):
"""
Loads test manifests and runs tests.
Expand Down Expand Up @@ -95,18 +108,7 @@ def main(self):
test_targets = self.options.tests
else:
# default to find known sibling test dirs
test_targets = []
sibling_dirs = [
'../json-ld-api/tests/',
'../json-ld-framing/tests/',
'../normalization/tests/',
]
for dir in sibling_dirs:
if os.path.exists(dir):
print('Test dir found', dir)
test_targets.append(dir)
else:
print('Test dir not found', dir)
test_targets = default_test_targets()

# ensure a manifest or a directory was specified
if len(test_targets) == 0:
Expand Down