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

Remove any data attributes not used in the "YAML tests" #4817

Merged
merged 2 commits into from
Jun 28, 2022

Conversation

anderseknert
Copy link
Member

Fixes #4813

Apologies in advance to whoever reviews this 🙃😄 Perhaps passing tests are enough here, I don't know.

Note that in a few files the order of the attributes (as printed) got changed, but not in any way that matters.

Signed-off-by: Anders Eknert anders@eknert.com

Fixes open-policy-agent#4813

Signed-off-by: Anders Eknert <anders@eknert.com>
@anderseknert anderseknert force-pushed the remove-yaml-test-cruft branch from b8534fc to d646276 Compare June 27, 2022 20:12
@philipaconrad philipaconrad changed the title Remove any data attrubutes not used in the "YAML tests" Remove any data attributes not used in the "YAML tests" Jun 27, 2022
@tsandall tsandall requested a review from srenatus June 27, 2022 21:49
@srenatus
Copy link
Contributor

I guess I'd rather review what got us here, can you share your script? Maybe put it in a gist and reference it in the X commit message, too?

@anderseknert
Copy link
Member Author

@srenatus https://gist.github.com/anderseknert/c4a1b2e7135bf8635f6f2dbb06f12e34

there’s a fair amount of manual fixes too though, both to account for the opa deps bugs, and in some cases keys that lost their string type and for unknown reason got evaluated… so not sure how useful that is compared to the actual end result.

Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

Can't review this thoroughly, but I appreciate the clean up. I think our best approach is this:

  1. merge this
  2. when dealing with any of these files, and something seems terribly odd, remember the cleanup

Also, the dependent projects using these files, like npm-opa-wasm, would perhaps also note if something is wonky in one of those files.

However, I'd first like to fix the pipeline issue before merging this. I'll push the button when ready.

@anderseknert
Copy link
Member Author

@srenatus SGTM! Will test this in Jarl as well, which runs about 850 of the tests at the moment.. and where the data fed into each of the tests account for something like half of the 13000 lines of code from the generated tests :D

@srenatus
Copy link
Contributor

@srenatus srenatus merged commit dfd4be4 into open-policy-agent:main Jun 28, 2022
@anderseknert anderseknert deleted the remove-yaml-test-cruft branch February 11, 2025 19:03
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.

Remove unused data from the YAML test suite files
2 participants