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

Impl test report #540

Closed
wants to merge 12 commits into from
Closed
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: 5 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
@@ -1 +1,5 @@
TODO
# TODO

.vscode

report/*/
6 changes: 6 additions & 0 deletions report/Dockerfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
ARG IMPLEMENTATION

FROM ${IMPLEMENTATION}

RUN apt-get update && \
apt-get install jq -y
49 changes: 49 additions & 0 deletions report/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
# Test Suite Runner

This folder contains scripts and docker definitions that can run the suite against an implementation. The implementations are configured in _implementations.json_.

## Add your implementation

Setting up your application requires several steps.

### Create a CLI

To add your implementation, you'll need to start by creating a CLI application that takes the following parameters:

- `--schema <PATH#POINTER>` - The relative URI to the schema, e.g. `../tests/draft2019-09/additionalItems.json#/0/schema`.
Copy link
Member

@karenetheridge karenetheridge Jan 9, 2022

Choose a reason for hiding this comment

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

The implication here is that the instance and schema are expected to be files in JSON format. Are any other encoding formats allowed or expected to be supported? (e.g. YAML?) Must these be files on the local filesystem, or are network-reachable URIs possibly going to be used as well?

Copy link
Member

Choose a reason for hiding this comment

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

e.g. is this legal? --schema https://json-schema.org/draft/2020-12/schema

Copy link
Member Author

Choose a reason for hiding this comment

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

These URIs point directly to the file and schema/instance inside of it. The docker-compose is copying the test suite as it exists in this repo directly into the container. Your CLI will need to find the file, open it, and navigate to the location indicated by the pointer fragment. The resulting JSON data will be the schema or instance as appropriate.

Copy link
Member Author

Choose a reason for hiding this comment

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

e.g. is this legal? --schema https://json-schema.org/draft/2020-12/schema

I expect that could be accepted by your CLI, but the test suite won't submit that. It'll only submit schemas that exist in the test suite.

Copy link
Member

Choose a reason for hiding this comment

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

These URIs point directly to the file and schema/instance inside of it. The docker-compose is copying the test suite as it exists in this repo directly into the container. Your CLI will need to find the file, open it, and navigate to the location indicated by the pointer fragment.

Great, please add that to the README.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had intended to add a "how it works" section that would include this.

- `--instance <PATH#POINTER>` - The relative URI to the data instance, e.g. `../tests/draft2019-09/additionalItems.json#/0/tests/0/data`.
Copy link
Member

@Julian Julian Feb 3, 2022

Choose a reason for hiding this comment

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

I think it'd be easier (and more future compatible) if the interface was JSON in and out, rather than CLI arguments, no?

Specifically, implementers wouldn't need to do their own URI resolution if we instead had the runner harness pipe a JSON object with schema and instance in it to their stdin.

And on the result side, we may at some point need information back from the CLI, so rather than using exit code, maybe we should instead immediately use {"result": "valid"} and {"result": "invalid"}, this way at some point we could have {"result": "invalid", "known_bug": "myrepo/issues/123"} or whatever.

Copy link
Member

Choose a reason for hiding this comment

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

Doing it this way would also mean we could specify that CLIs should accept JSON terminated by line delimiters so we don't pay startup cost of each CLI repeatedly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Doing it this way would also mean we could specify that CLIs should accept JSON terminated by line delimiters so we don't pay startup cost of each CLI repeatedly.

We'd have to minimize all of the test files to remove line delimiters. I'm not sure I can do that in a bash script.

Given that I'm learning as I go, all of this is a tall order.

Copy link
Member

Choose a reason for hiding this comment

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

Do you mean newlines?
Two NL in a row could be used to signal the end of a json blob, rather than removing all newlines and passing the data as a single line.

- `--spec-version <VERSION>` - The draft/version for the schema, e.g. `draft2020-12`. The value for this parameter will match a folder name under `/tests/` in this repo.
<!-- Do we need this parameter? -->
Copy link
Member

Choose a reason for hiding this comment

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

I can't see why we'd need it, so maybe kill it until someone asks for it, though adding it would be easier with the above.

Copy link
Member Author

Choose a reason for hiding this comment

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

We may need to to configure the optional format tests. Some validators might not have that enabled and we could use this as a signal to the implementation to enable it.

For draft 2020-12 we could use a custom format-required meta-schema, but that wouldn't work for the other drafts.

Copy link
Member

Choose a reason for hiding this comment

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

For an initial proof-of-concept I'd leave out all optional tests and stick to just what the specification requires.

Later on, we'll have to think about what config options would be required (and how to signal them to the application) for the various option tests, as there's a mixed bag of a bunch of them in there. (or, take the opportunity to rethink our organization of optional tests, since this keeps coming up as a point of confusion and we're being rather informal about the whole thing right now.) e.g. some tests require format validation to be supported and turned on; some tests require bignum support; some tests require non-standard regex syntax, etc. We may also want to add more optional tests later on for testing annotation collection, or the operation of the content* keywords, etc etc.

<!-- - `--validate-formats` - Indicates whether to validate formats. -->

Internally, you may invoke your implementation as you see fit, setting any options you need to produce the correct outcome.

Validation outcome is indicated by the CLI exit code as follows:

- **-1** or **255** - The instance was determined to be invalid against the schema.
- **0** - The instance was determined to be valid against the schema.
- **1** - An application error occurred preventing the schema or instance from being processed.
- **2** - The scenario is not supported.

Each of these will be represented in the final report.

### Package your CLI

The CLI will need to be packaged into a Linux Docker image.

At a minimum this docker image will need your CLI as well as any runtime needed.

You'll need to push this to a public repository such as Docker Hub so that it can be pulled when the test suite runs.

### Update this repository

Lastly, update a file in this repo.

_implementations.json_ (in this folder) needs an entry that includes the following:

- `docker-image` - The name of your docker image. The `latest` tag will be used.
- `command` - The command to run your CLI.
- `versions` - An array of versions/drafts supported by your implementations. (To save time, unsupported versions will be skipped.)
Copy link
Member

Choose a reason for hiding this comment

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

It seems like this may cause a rush of PRs when new drafts are released as each person adds the new draft to the file.

Maybe instead we indeed should send the draft version, and specify that the implementer's CLI needs to return an exit status that indicates to skip the draft if it doesn't recognize it.

Copy link
Member

Choose a reason for hiding this comment

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

That sounds good to me. It would be nice for things to adapt automatically as we add a new spec version or an implementation extends support to a new version in their 'latest' image.


Once added, the runner will do the rest.

10 changes: 10 additions & 0 deletions report/docker-compose.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
version: "3.9"
services:
test-suite:
image: test-run
volumes:
- .:/report
- ./${IMPLEMENTATION}:/output
- ../tests:/tests
- ../remotes:/remotes
entrypoint: /report/run-suite.sh ${IMPLEMENTATION}
7 changes: 7 additions & 0 deletions report/implementations.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[
{
"docker-image": "gregsdennis/json-everything",
"command": "dotnet json-everything.dll",
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be an array to avoid the off-chance of needing shell quoting.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand.

Copy link
Member

Choose a reason for hiding this comment

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

(I deleted my previous "I agree that this doesn't make sense" comment, after thinking about it harder) :)

If a command is provided as a single string, then the implication is it's passed to the shell, which may parse some characters unless they're escaped; if the command is passed as an array of words, then we can bypass the shell entirely and just hand it to exec (or equivalent).

e.g. perl's exec and system functions pass the command through the shell if one argument is provided, but skips that step if multiple arguments are provided, with the presumption that tokenization has already been done (and without having to fuss with escaping characters that the shell might do something with). https://perldoc.perl.org/functions/exec

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay. Again. I'm learning as I go here, so feel free to slow am example or add a commit. I don't know how to translate a string array into a bash command.

Copy link
Member

Choose a reason for hiding this comment

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

That's what I was referring to yeah -- and no worries at all! I can leave some commits showing what I mean -- it's true, shell scripting indeed doesn't make some of this trivial.

"versions": ["draft6", "draft7", "draft2019-09", "draft2020-12"]
}
]
6 changes: 6 additions & 0 deletions report/report-template.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# This file was generated by running `/report/run-report.sh ${IMPLEMENTATION}`

# This is the compliance report for the indicated implementation

implementation: ${IMPLEMENTATION}
results:
16 changes: 16 additions & 0 deletions report/run-report.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
#!/bin/sh

export IMPLEMENTATION=${1}

echo "Running test suite with '${1}'"

docker image rm test-run -f

docker build -t test-run --build-arg IMPLEMENTATION=${1} .

docker create --name test-run test-run

docker-compose run test-suite

# docker-compose logs --no-color > docker-compose.log
# [[ -z "${failed:-}" ]] || exit 1
75 changes: 75 additions & 0 deletions report/run-suite.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
#!/bin/bash

echo "Running test suite with implementation ${1}"

command=$(jq -r ".[] | select(.\"docker-image\"==\"${1}\") | .command" /report/implementations.json)
versions=$(jq -r ".[] | select(.\"docker-image\"==\"${1}\") | .versions" /report/implementations.json)
output="/output/result.yml"

sed -e "s~\${IMPLEMENTATION}~${1}~" /report/report-template.yml > ${output}

if [[ "${command}" == "" ]]
then
echo "Implementation either not found or not configured with a command to execute"
exit 1
fi

for d in /tests/*/
do
specVersion=$(basename $d)

if ! [[ ${versions[@]} =~ $specVersion ]]
then
# record that the draft isn't supported
continue
fi

for f in $(find $d) # $d contains the ending /
do
filename=$(basename $f)
if [[ $f == *"optional"* ]]
then
optional=true
else
optional=false
fi

scenarioIndex=0
jq -cr '.[]' < $f | while read j
do
scenario=$(echo $j | jq -r '.description')
schemaUri="$f#/$scenarioIndex/schema"
caseIndex=0
echo $j | jq -c '.tests[]' | while read c
do
case=$(echo $c | jq -r '.description')
instanceUri="$f#/$scenarioIndex/tests/$caseIndex/data"
case "$(echo $c | jq '.valid')" in
true) expected="valid" ;;
false) expected="invalid" ;;
esac

${command} --schema $schemaUri --instance $instanceUri --spec-version $specVersion
evaluated=$?

case $evaluated in
255) result="invalid" ;;
0) result="valid" ;;
1) result="error" ;;
2) result="unsupported" ;;
esac

echo " - draft: ${specVersion}" >> $output
echo " file: ${filename%%.*}" >> $output
echo " scenario: ${scenario}" >> $output
echo " case: ${case}" >> $output
echo " optional: ${optional}" >> $output
echo " expected: ${expected}" >> $output
echo " result: ${result}" >> $output

((caseIndex++))
done
((scenarioIndex++))
done
done
done