Skip to content

tests: API: Test unrecognized fields everywhere #1382

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

jku
Copy link
Member

@jku jku commented May 10, 2021

Currently we test adding unrecognised fields in specific places in our
file format structure. Instead add the fields everywhere (except
specific locations where that would not be valid).

Signed-off-by: Jussi Kukkonen [email protected]


The idea here is to test inserting custom items into every dictionary (except where spec doesn't actually allow it), instead of testing only specific dictionaries.

@jku jku requested a review from MVrachev May 10, 2021 13:49
Currently we test adding unrecognised fields in specific places in our
file format structure. Instead add the fields everywhere (except
specific locations where that would not be valid).

Signed-off-by: Jussi Kukkonen <[email protected]>
@jku jku force-pushed the automate-unrecognised-fields-testing branch from 29b9e92 to dad0b58 Compare May 12, 2021 05:59
@jku
Copy link
Member Author

jku commented May 12, 2021

rebased on develop


# if dicts keys are limited in spec or if dicts values are
# strictly defined, skip it
if name in ["hashes", "keys", "meta", "roles", "targets"]:
Copy link
Contributor

@sechkova sechkova May 12, 2021

Choose a reason for hiding this comment

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

Not really a review but how did we come up with this list? I guess you did it by going through the spec. Can we note it somewhere so that we don't do it again?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes you guessed correctly. I'll try to phrase that somehow clearly

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wondering why do you include meta here?

Copy link
Member Author

@jku jku May 14, 2021

Choose a reason for hiding this comment

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

because it seems to me the meta content is strictly defined: keys must be metadata filenames (in the timestamp case there is basically only one allowed value). values must be MetaFile dict representations. I don't think allowing something else is reasonable.

Copy link
Collaborator

@MVrachev MVrachev left a comment

Choose a reason for hiding this comment

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

Overall, it checks what I expected from such a test.
I had a look into the spec if you missed a dictionary where we don't want to have unrecognized_fields and it seems there are no other.

Only one unclear thing for me which I asked in a comment.

@MVrachev
Copy link
Collaborator

I will suggest combing this pr with changes in ADR 8 clarifying that some of the fields won't support unrecognized fields.
We want to have that documented to minimize the chance of including unrecognized fields where it's sensitive.

@jku
Copy link
Member Author

jku commented May 28, 2021

The alternative to this PR would be to include a test case "unrecognised fields" in every test dataset in #1416 ... That would mean none of this code is needed (good) but does mean we'd have to be diligent about really including the cases everywhere (bad)

We could leave this as draft until we see if we get #1416 implemented in a nice way instead?

@MVrachev
Copy link
Collaborator

Yes, I suppose let's agree on 1416 and then decide about this one.

@jku jku marked this pull request as draft June 21, 2021 08:55
@jku
Copy link
Member Author

jku commented Jun 21, 2021

made this a draft until we see if serialization tests can/should cover this without code

@MVrachev
Copy link
Collaborator

MVrachev commented Jun 23, 2021

The Pr using the table testing for tests connected with unrecognized fields out in the wild: #1466.

@jku jku closed this Jul 5, 2021
@jku jku deleted the automate-unrecognised-fields-testing branch December 30, 2024 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants