-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-15668: Simplified skip logic in integration tests #12409
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
|
|
pitrou
left a comment
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.
This looks cool! Here are some comments and questions.
dev/.gitignore
Outdated
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.
Why not reuse the same .venv convention as above?
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.
both are common uses, right?
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.
That's true. If this is for personal use you may want to broaden it to venv*/, then.
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.
This could also go into a single file languages.yaml for all languages, what do you think?
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.
good idea. Will do that.
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.
imo it makes it a bit easier to follow up on when a specific language changes if they are separate files.
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.
No problem, let's keep it like this then.
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.
So the "unsupported golden" entry in the YAML file isn't used (yet)?
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.
exactly, I tried to cover one portion of the cases in this PR; there are 2 missing to make them part of the configuration:
- golden files
- flight
this change was only to be able to decouple the names of the golden files from the name of the language to language tests.
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 see, do you plan to tackle those items in another PR?
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.
yeap, that was the idea - have all language capabilities described in a configuration
|
Closing because it has been untouched for a while, in case it's still relevant feel free to reopen and move it forward 👍 |
This PR simplifies the logic associated to integration tests by lifting the declaration of what is supported and not supported by each language to a
yamlfile. This allows for a better overview of what is supported and not, as well as a clearer separation between