-
Notifications
You must be signed in to change notification settings - Fork 1
support skipping last node by index if parent node is a list #16
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
Conversation
@@ -218,7 +220,7 @@ def _assert_all( | |||
self.skip_verification_paths = skip_verification_paths or [] | |||
if skip_verification_paths: | |||
SNAPSHOT_LOGGER.warning( | |||
f"Snapshot verification disabled for paths: {skip_verification_paths}" | |||
"Snapshot verification disabled for paths: %s", skip_verification_paths |
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.
see https://docs.astral.sh/ruff/rules/logging-f-string/
note: could also be added to the project rules as we did for LocalStack
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.
👍 Added #19 for this.
22179f0
to
60834ef
Compare
60834ef
to
c827c51
Compare
Sorry, the PR got closed with the automatic linked of GitHub with another PR. I didn't know it also worked with PRs and not only issues 🤔 |
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.
Thank you @bentsku! I only committed a small naming change. The rest looks good, thanks for great test coverage 👍
@@ -25,6 +25,8 @@ | |||
SNAPSHOT_LOGGER = logging.getLogger(__name__) | |||
SNAPSHOT_LOGGER.setLevel(logging.DEBUG if os.environ.get("DEBUG_SNAPSHOT") else logging.WARNING) | |||
|
|||
_SKIP_PLACEHOLDER_VALUE = "$__to_be_skipped__$" |
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 changed to a more specific name and value here, for the sake of readability in code and in debugging.
@@ -218,7 +220,7 @@ def _assert_all( | |||
self.skip_verification_paths = skip_verification_paths or [] | |||
if skip_verification_paths: | |||
SNAPSHOT_LOGGER.warning( | |||
f"Snapshot verification disabled for paths: {skip_verification_paths}" | |||
"Snapshot verification disabled for paths: %s", skip_verification_paths |
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.
👍 Added #19 for this.
I've encountered the issue in 2 separates occasions, where I could not set a snapshot skip if the value I wanted to escape was an item inside a list, and it was the last "node".
For example, a list in a snapshot would look like:
And we would a different value for the
"value2"
item, settingpaths=["$..test[1]"]
would not be skipped, it was ignored by our "skip_snapshot_verify" utility.See those 2 PRs, where I had to transform the list returned into a dictionary so I could skip by index:
Sometimes, we'd just skip the whole list because of the issue like in localstack/localstack#12831, which took the snapshot skip for existing tests in the file.
For example, skipping a dictionary key from a dictionary living inside a list, but only in one list item would work, if the index as part of the path but not the last part.
I've tried to implemented and quickly realized why we had not done it yet. If you refer to a value by index, and then mutate said list to remove it, the next skip paths will refer to a wrong index, as the list has shifted.
One solution to this is to replace those values we want to remove by placeholders, and remove them all at once. This way, we're not dealing with indexes anymore.
Not sure if it's the cleanest solution, the
_remove_skip_verification_paths
is starting to be a little bit crowded.I also thought about updating the reporting to have better suggestion around indexes instead of suggesting recursive descent pretty often, but this is another can of worms with deduplication and reasoning about the diff (what if we have 8 items that are wrong in an array of 10 items, do we want to suggest to individually skip the 8 items, each in their own line? I believe we could use the
[index1,index2,…]
format to skip multiple indexes, but it looks our JSONPath library does not support it. But again, this could be tackled in a follow up).At least this PR enables better JSONPath support in our skip paths for "advanced" usage.