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

Fix template projects example tests #4159

Merged
merged 3 commits into from
Sep 16, 2024

Conversation

Manezki
Copy link
Contributor

@Manezki Manezki commented Sep 12, 2024

Description

New project created by kedro new --tools test has example test that does not work.

Development notes

  • Changed ConfigLoader to the newly embraced OmegaConfigLoader.
  • Added env argument to KedroContext initialization.

After the changes a new project can successfully run tests with pytest .

Checklist

  • Read the contributing guidelines
  • Signed off each commit with a Developer Certificate of Origin (DCO)
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the RELEASE.md file
  • Added tests to cover my changes
  • Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

Signed-off-by: Janne Holopainen <manezki@gmail.com>
Signed-off-by: Janne Holopainen <manezki@gmail.com>
Signed-off-by: Janne Holopainen <manezki@gmail.com>
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.

Thank you so much @Manezki ! 🌟 I can't believe we didn't spot that earlier 🙈

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.

Thanks @Manezki!

@Manezki
Copy link
Contributor Author

Manezki commented Sep 14, 2024

Thanks for the snappy review (and the positive comments).

I don't have permission to merge the PR. Is there still something I should do?
For the remaining "open points" I have the following:

  • The failure of the language linter is a false negative on my alias.
  • I haven't checked the Kedro-Viz impact, as I have no idea about the surface I would have to cover - but intuitively it would make sense that there's no impact.
  • I haven't added a test-case for the error as that would probably require calling pytest from the tests. I'm not sure if the test harness would support that. Should I still look into it?

@merelcht
Copy link
Member

merelcht commented Sep 16, 2024

Thanks for the snappy review (and the positive comments).

I don't have permission to merge the PR. Is there still something I should do?

No, I can merge it for you 👍

For the remaining "open points" I have the following:

  • The failure of the language linter is a false negative on my alias.

This is more a suggestion, but in this case we can ignore it.

  • I haven't checked the Kedro-Viz impact, as I have no idea about the surface I would have to cover - but intuitively it would make sense that there's no impact.

Indeed, no impact on Viz for this change.

  • I haven't added a test-case for the error as that would probably require calling pytest from the tests. I'm not sure if the test harness would support that. Should I still look into it?

No need for that.

Thanks again for the contribution!

@merelcht merelcht merged commit 91765e3 into kedro-org:main Sep 16, 2024
39 of 40 checks passed
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