fix(tests): Fix false negative cram tests #1666
Conversation
…ff_jsons.py invocations
…eshadfield @huddlej can you double check this is intended?
…ems to have caused no issues so far so likely ok
tests/functional/ancestral/data/ancestral_mutations_with_root_sequence.json
Outdated
Show resolved
Hide resolved
corneliusroemer
requested review from
jameshadfield,
huddlej,
victorlin and
tsibley
| # Test for most fatal errors in regex path usage | ||
| # Exclude regexes should never match `'`, otherwise the diff is always going to pass |
There was a problem hiding this comment.
Should this include " as well? I'm assuming root['generated_by'] and root["generated_by"] are both valid.
There was a problem hiding this comment.
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.
I just tested with both ['seqid'] and ["seqid"]:
$ python3 "$TESTDIR/../../../../scripts/diff_jsons.py" "$TESTDIR/../data/clades.json" clades_no-labels.json --exclude-regex-paths "['seqid']"
- {'dictionary_item_removed': [root['branches']]}
+ Traceback (most recent call last):
+ File "/Users/vlin/Dropbox/repos/nextstrain/augur/tests/functional/clades/cram/../../../../scripts/diff_jsons.py", line 30, in <module>
+ raise Exception(
+ Exception: Exclude regex ['seqid'] matches `'` which means this diff will always pass which is probably not what you want.
+ You probably forgot to escape something in your regex. See for example: https://stackoverflow.com/a/79173188/7483211
+ [1] $ python3 "$TESTDIR/../../../../scripts/diff_jsons.py" "$TESTDIR/../data/clades.json" clades_no-labels.json --exclude-regex-paths '["seqid"]'
- {'dictionary_item_removed': [root['branches']]}
+ {}It looks like " should get the same treatment.
There was a problem hiding this comment.
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.
| 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.
nit
Adding or [] (e596b50) doesn't seem useful:
- We don't run mypy/pyright on this file
- A better solution would be to set
default=[]onadd_argument()
There was a problem hiding this comment.
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.
Why is changing the defaults better?
Setting the default up top allows for a valid assumption that args.exclude_regex_paths is always iterable. Otherwise we would have to do another args.exclude_regex_paths or [] if using it as an iterable somewhere else in the code for whatever reason.
Co-authored-by: Victor Lin <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1666 +/- ##
==========================================
+ Coverage 72.26% 72.29% +0.02%
==========================================
Files 79 79
Lines 8272 8276 +4
Branches 1691 1691
==========================================
+ Hits 5978 5983 +5
+ Misses 2009 2008 -1
Partials 285 285 ☔ View full report in Codecov by Sentry. |
|
This might be worth an internal changelog entry |
Description of proposed changes
diff_jsons.pywhen exclude-regex-path is used incorrectly like in issue 1665 to prevent this issue from reoccurring silently["root['generated_by']", "root['meta']['updated']"]instead of["root['generated_by']['version']"]. This allows simplification of a lot of invocations ofdiff_jsons.pyand in turn reduces chance of errors similar to issue 1665. Anything withingenerated_bycan be ignored by cram tests,meta.updatedis only used in export, but no harm excluding by default as it isn't used in any other tests. If there's opposition, happy to revert that change.Checklist