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

Move tools e2e tests to kedro-starters #209

Merged
merged 8 commits into from
Jan 18, 2024

Conversation

SajidAlamQB
Copy link
Contributor

@SajidAlamQB SajidAlamQB commented Jan 18, 2024

Motivation and Context

related to: kedro-org/kedro#3518

After discussing with @merelcht, we've concluded that it would be better to relocate some of the e2e tests for tools to the kedro-starters repository. This decision follows a recent update in Kedro, which ensures that the example starters pulled via tools will always match the current version of Kedro. This change has inadvertently led to release delays, as kedro-starters are released after a kedro.

To solve this issue, we moved/copied some of the tests to kedro-starters. However, this does means that we will no longer run the pipelines within Kedro for the --tools e2e tests. instead, these tests will occur in the starters repository. Given all this we still think its necessary for smoother release processes.

Would love to get everyone's input on this. @merelcht, @AhdraMeraliQB, @ankatiyar, @noklam, @DimedS, @lrcouto.

Checklist

  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Assigned myself to the PR
  • Added tests to cover my changes

Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

Woohoo all tests passing 🥳 I'm happy with these tests being here instead of kedro.

Copy link
Contributor

@ankatiyar ankatiyar left a comment

Choose a reason for hiding this comment

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

LGTM!

@ankatiyar
Copy link
Contributor

ankatiyar commented Jan 18, 2024

Should we add nightly/maybe less frequent builds here as well? Since PRs on kedro-starters are less frequent? @SajidAlamQB @merelcht

@merelcht
Copy link
Member

Should we add nightly/maybe less frequent builds here as well? Since PRs on kedro-starters are less frequent? @SajidAlamQB @merelcht

Yes that's a good idea. Nightly seems like a good cadence and that's the same as we do for the plugins right?

@DimedS
Copy link
Contributor

DimedS commented Jan 18, 2024

Thank you, @SajidAlamQB, for trying various methods to solve that issue. I have a query regarding the separation of tests from our main repository to the starters repository: would it make sense to initiate a check of the starters repository whenever a pull request is made on the main repository? Is such an integration feasible as a part of check on main repo before allowing merge?

@SajidAlamQB
Copy link
Contributor Author

Thank you, @SajidAlamQB, for trying various methods to solve that issue. I have a query regarding the separation of tests from our main repository to the starters repository: would it make sense to initiate a check of the starters repository whenever a pull request is made on the main repository? Is such an integration feasible as a part of check on main repo before allowing merge?

It definitely should be I believe we do it for viz.

@SajidAlamQB
Copy link
Contributor Author

Adding these tests has increased the runtime by ~7 minutes.

@SajidAlamQB SajidAlamQB merged commit 3783891 into main Jan 18, 2024
19 checks passed
@SajidAlamQB SajidAlamQB deleted the move-tools-e2e-test-to-starters- branch January 18, 2024 18:49
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.

4 participants