Skip to content
This repository has been archived by the owner on Sep 17, 2024. It is now read-only.

Add CI #2

Closed
MarcoGorelli opened this issue Aug 29, 2024 · 5 comments · Fixed by narwhals-dev/narwhals#899
Closed

Add CI #2

MarcoGorelli opened this issue Aug 29, 2024 · 5 comments · Fixed by narwhals-dev/narwhals#899
Labels
ci good first issue Good for newcomers

Comments

@MarcoGorelli
Copy link
Member

It would be good to add CI to this repo, which follows the steps on the README and checks that the queries actually execute

@MarcoGorelli MarcoGorelli added good first issue Good for newcomers ci labels Aug 29, 2024
@IsaiasGutierrezCruz
Copy link
Contributor

It's a good idea. I can take this task c:

@MarcoGorelli
Copy link
Member Author

awesome, thanks @IsaiasGutierrezCruz !

@IsaiasGutierrezCruz
Copy link
Contributor

Hi @MarcoGorelli!

I was finishing the implementation of this validation, but then I realized that you decided to store the logic of the queries in the main repo, haha.

I implemented a test to validate that the queries actually execute and a workflow exclusively for the tpch directory. Could you please give me your feedback on the implementation? I thought that implementing the validation only in the tpch directory is better than including it in the general tests for narwhals, but I don't know if you agree c:

Link of the branch: https://github.com/IsaiasGutierrezCruz/narwhals/tree/dev/queries_validation

@MarcoGorelli
Copy link
Member Author

ah sorry about that 😄

looks great, thanks! 🙏

if these tests would be quite long, maybe we could just execute them when we label PRs with "full-test"? https://stackoverflow.com/a/62331521/4451315

@IsaiasGutierrezCruz
Copy link
Contributor

Yes, that sounds great. I already implemented it in this pull request 😃: narwhals-dev/narwhals#899

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ci good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants