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

Replace validation testing with JSON Schema v7 compatible libraries #499

Merged
merged 34 commits into from Feb 11, 2020

Conversation

@acmiyaguchi
Copy link
Contributor

acmiyaguchi commented Feb 8, 2020

This will fix #332, and also introduces the jsonschema python package in case testing is done locally. There are 3 failing tests, 1 in python and 2 in java.

I've tested this on my local machine (without java configured, because pyjnius was not playing well with my macOS instance) which will skip the java tests, and in the docker image which can run either. When running in docker, I recommend the -n flags.

command stats
local, pytest 1 failed, 166 passed, 167 skipped in 8.25s
scripts/mps-test -k python 1 failed, 166 passed, 167 deselected in 157.60s (0:02:37)
scripts/mps-test -k java 2 failed, 165 passed, 167 deselected in 141.69s (0:02:21)

The scripts/mps-build and scripts/mps-test are helpers for this workflow.

Current errors:

test_validation_pass_python[glean/glean.1.baseline.pass.json]
E           jsonschema.exceptions.ValidationError: 'examples.labeled_timing_distribution_example' does not match '^[a-z_][a-z0-9_]{0,29}(\\.[a-z_][a-z0-9_]{0,29})+$'
E           
E           Failed validating 'pattern' in schema['properties']['metrics']['properties']['labeled_timing_distribution']['propertyNames']:
E               {'maxLength': 61,
E                'pattern': '^[a-z_][a-z0-9_]{0,29}(\\.[a-z_][a-z0-9_]{0,29})+$',
E                'type': 'string'}
E           
E           On instance['metrics']['labeled_timing_distribution']:
E               'examples.labeled_timing_distribution_example'
test_validation_pass_java[glean/glean.1.baseline.pass.json]
E   jnius.JavaException: JVM exception occurred: #/metrics/labeled_timing_distribution/examples.labeled_timing_distribution_example: string [examples.labeled_timing_distribution_example] does not match pattern ^[a-z_][a-z0-9_]{0,29}(\.[a-z_][a-z0-9_]{0,29})+$
test_validation_pass_java[telemetry/untrusted-modules.4.unknownmodule.pass.json]
E   jnius.JavaException: JVM exception occurred: #/payload/combinedStacks/stacks/0/2/1: expected type: Number, found: String

There are still a few more things to do, but this should be in a reviewable state.

Checklist for reviewer:

  • Commits should reference a bug or github issue, if relevant
  • Scan the PR and verify that no changes (particularly to .circleci/config.yml) will cause environment variables (particularly credentials) to be exposed in test logs
  • Trigger the integration CI test by pushing this revision as discussed in the README and review the report posted in the comments.

For glean changes:

  • Update include/glean/CHANGELOG.md
@acmiyaguchi acmiyaguchi requested a review from jklukas Feb 8, 2020
@auto-assign auto-assign bot requested a review from relud Feb 8, 2020
@acmiyaguchi acmiyaguchi removed the request for review from relud Feb 8, 2020
tests/test_validation.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
Copy link
Contributor

jklukas left a comment

Note that the Java validator will default to json schema v4 unless the schema specifies a draft via the $schema keyword or the validator is built with specific options to control the draft in use. It looks like what we're doing here matches the configuration in ingestion-beam, so that's good. Just noting that this PR does not mean we are actually using draft 7 yet. We can address migrating versions later.

I'm really happy to see that it's possible to use the Java library from python in this way. Well done in finding the machinery to make that happen.

So, this approach looks great. I'd recommend tagging @relud for additional eyes on the pytest code.

README.md Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
pom.xml Show resolved Hide resolved
acmiyaguchi and others added 4 commits Feb 10, 2020
Co-Authored-By: Jeff Klukas <jeff@klukas.net>
@acmiyaguchi acmiyaguchi requested review from relud and jklukas Feb 10, 2020
@acmiyaguchi

This comment has been minimized.

Copy link
Contributor Author

acmiyaguchi commented Feb 10, 2020

@relud r?, in particular with respect to the pytest configuration for generating tests.

tests/test_validation_java.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

jklukas left a comment

I love it.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
pom.xml Show resolved Hide resolved
tests/test_validation_java.py Outdated Show resolved Hide resolved
tests/test_validation_java.py Outdated Show resolved Hide resolved
@dataops-ci-bot

This comment has been minimized.

Copy link

dataops-ci-bot commented Feb 11, 2020

Integration report for "Add missing objects in java tests"

Report for upstream
Report for latest commit

No changes detected.

@acmiyaguchi acmiyaguchi merged commit f8f7c08 into master Feb 11, 2020
3 checks passed
3 checks passed
ci/circleci: integrate Your tests passed on CircleCI!
Details
ci/circleci: post-artifacts Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
@acmiyaguchi acmiyaguchi deleted the python branch Feb 11, 2020
dataops-pipeline-schemas added a commit that referenced this pull request Feb 12, 2020
3bd20bf	2020-02-12 13:03:15 -0800	Ignore only build at root directory; add java ignores (#506)
115682f	2020-02-11 17:53:36 -0800	Fix CI on master (#504)
815522b	2020-02-12 02:37:35 +0100	Bug 1602463 - Add a schema for the new default-browser ping (#495)
f8f7c08	2020-02-11 14:28:36 -0800	Replace validation testing with JSON Schema v7 compatible libraries  (#499)
457f05e	2020-02-11 12:19:49 -0800	Add validation test for null devices in syncs
7c8aa57	2020-02-11 12:19:49 -0800	Bug 1614418 allow null for syncs device versions
4017e78	2020-02-11 11:58:35 -0800	Add initial GRAVEYARD document (#502)
acmiyaguchi pushed a commit that referenced this pull request Feb 13, 2020
3bd20bf	2020-02-12 13:03:15 -0800	Ignore only build at root directory; add java ignores (#506)
115682f	2020-02-11 17:53:36 -0800	Fix CI on master (#504)
815522b	2020-02-12 02:37:35 +0100	Bug 1602463 - Add a schema for the new default-browser ping (#495)
f8f7c08	2020-02-11 14:28:36 -0800	Replace validation testing with JSON Schema v7 compatible libraries  (#499)
457f05e	2020-02-11 12:19:49 -0800	Add validation test for null devices in syncs
7c8aa57	2020-02-11 12:19:49 -0800	Bug 1614418 allow null for syncs device versions
4017e78	2020-02-11 11:58:35 -0800	Add initial GRAVEYARD document (#502)
023537f	2020-02-10 09:31:40 -0400	bug 1604312 - Scalars can now appear in 'deletion-request' pings. (#481)
fb3c473	2020-02-07 16:43:00 -0500	error schema updates based on review
30947fe	2020-02-07 16:43:00 -0500	Bug 1613225 Add descriptions to payload_bytes_* fields
829c82e	2020-02-05 14:14:28 -0800	Bug 1595063 - Add ua attribution to environment data (#494)
63dcb42	2020-02-03 16:49:19 -0500	anyOf -> oneOf
0739fed	2020-02-03 16:49:19 -0500	Add CHANGELOG
36c965f	2020-02-03 16:49:19 -0500	1612940: Allow "null" extras in experiment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.