-
Notifications
You must be signed in to change notification settings - Fork 128
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
fix(tests): Fix false negative cram tests #1666
base: master
Are you sure you want to change the base?
Changes from 11 commits
8aa5ead
e596b50
21d2227
7c99d29
75c7976
9327a7d
11587e5
bd8d892
ba3c43f
6b55555
3dacd2d
27d18b4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ | |
import argparse | ||
import deepdiff | ||
import json | ||
import re | ||
|
||
from augur.argparse_ import ExtendOverwriteDefault | ||
|
||
|
@@ -15,12 +16,22 @@ | |
parser.add_argument("first_json", help="first JSON to compare") | ||
parser.add_argument("second_json", help="second JSON to compare") | ||
parser.add_argument("--significant-digits", type=int, default=5, help="number of significant digits to use when comparing numeric values") | ||
parser.add_argument("--exclude-paths", nargs="+", action=ExtendOverwriteDefault, help="list of paths to exclude from consideration when performing a diff", default=["root['generated_by']['version']"]) | ||
parser.add_argument("--exclude-paths", nargs="+", action=ExtendOverwriteDefault, help="list of paths to exclude from consideration when performing a diff", default=["root['generated_by']", "root['meta']['updated']"]) | ||
parser.add_argument("--exclude-regex-paths", nargs="+", action="extend", help="list of path regular expressions to exclude from consideration when performing a diff") | ||
parser.add_argument("--ignore-numeric-type-changes", action="store_true", help="ignore numeric type changes in the diff (e.g., int of 1 to float of 1.0)") | ||
|
||
args = parser.parse_args() | ||
|
||
# Test for most fatal errors in regex path usage | ||
# Exclude regexes should never match `'`, otherwise the diff is always going to pass | ||
for regex in args.exclude_regex_paths or []: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit Adding
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just because we don't run mypy doesn't mean that we shouldn't prevent type errors from happening? The error actually happens irrespective of running mypy. It's not mypy erroring it's a real runtime error. Why is changing the defaults better? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Setting the default up top allows for a valid assumption that |
||
result = re.compile(regex).search("'") | ||
if result is not None: | ||
print( | ||
f"Exclude regex {regex} matches `'` which means this diff will always pass which is probably not what you want.\n" | ||
"You probably forgot to escape something in your regex. See for example: https://stackoverflow.com/a/79173188/7483211" | ||
) | ||
corneliusroemer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
with open(args.first_json, "r") as fh: | ||
first_json = json.load(fh) | ||
|
||
|
corneliusroemer marked this conversation as resolved.
Show resolved
Hide resolved
|
Large diffs are not rendered by default.
corneliusroemer marked this conversation as resolved.
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this include
"
as well? I'm assumingroot['generated_by']
androot["generated_by"]
are both valid.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the path that deep diff uses to match against uses a single quote not double quote. Hence excluding only single quote. Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tested with both
['seqid']
and["seqid"]
:It looks like
"
should get the same treatment.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, there's a bunch more things that should not not be allowed to be matched, things like
[
and]
and any single letter. But in a way it's not super clearcut as someone might want to exclude everything but a single path (they should use an include path instead then) - so I don't know how far we want to go here. Including"
sounds good though, no harm I can see.