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

Added tests for template generation #9

Merged
merged 12 commits into from
Jun 13, 2024
Merged

Conversation

santacodes
Copy link
Collaborator

Description

  • Added basic tests for template generation using pytest-cookies.
  • Added cookiecutter as a dependency and setup ___init.py__ for the src layout.

Copy link
Member

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

Thanks, @santacodes! Looks good to me. Could you please add a minimal CI job to check if the tests are working fine?

Copy link
Member

@arjxn-py arjxn-py left a comment

Choose a reason for hiding this comment

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

Thanks @santacodes, Good Start I must admit :)
Some comments below to propose some improvements

tests/test_project_generation.py Outdated Show resolved Hide resolved
tests/test_project_generation.py Show resolved Hide resolved
tests/test_project_generation.py Outdated Show resolved Hide resolved
tests/test_project_generation.py Show resolved Hide resolved
@santacodes
Copy link
Collaborator Author

Thanks, @santacodes! Looks good to me. Could you please add a minimal CI job to check if the tests are working fine?

Sorry for the delay @Saransh-cpp, I was travelling. I added CI for the tests, created a nox session for testing the template generation, and tested it on my branch. Could you please review and look at it? Also, I added uv to the nox backend, which seems to be faster.

Copy link
Member

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

Thanks, @santacodes! Few minor changes below -

noxfile.py Outdated Show resolved Hide resolved
src/pybamm_cookiecutter/__init__.py Outdated Show resolved Hide resolved
.github/test_on_push.yml Show resolved Hide resolved
@Saransh-cpp
Copy link
Member

TODO:

  • Update branch protection rule to include the new CI (status checks must pass before merging)

@santacodes
Copy link
Collaborator Author

TODO:

  • Update branch protection rule to include the new CI (status checks must pass before merging)

branchrule

I think I can only create a branch rule allowing merges only after status checks when the status checks themselves have been merged to the main branch.

@Saransh-cpp
Copy link
Member

Hi, don't worry about the rules. I'll set them up.

Saransh-cpp
Saransh-cpp previously approved these changes Jun 11, 2024
Copy link
Member

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks @santacodes! I'll wait for @agriyakhetarpal and @arjxn-py's reviews.

@Saransh-cpp Saransh-cpp requested a review from arjxn-py June 11, 2024 15:38
@arjxn-py
Copy link
Member

Hi, I'll review this in a couple of hours.

Copy link
Member

@arjxn-py arjxn-py left a comment

Choose a reason for hiding this comment

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

Looks much better @santacodes 🙂, Some comments below for minor improvements, feel free to ping me again.
Before we merge, we would want that the workflow starts running on this same PR.
Plus, I really appreciate that we're also adapting to uv in this.
xref PyBaMM#3825

tests/test_project_generation.py Outdated Show resolved Hide resolved
.github/test_on_push.yml Outdated Show resolved Hide resolved
tests/test_project_generation.py Outdated Show resolved Hide resolved
tests/test_project_generation.py Outdated Show resolved Hide resolved
tests/test_project_generation.py Show resolved Hide resolved
.github/test_on_push.yml Outdated Show resolved Hide resolved
.github/test_on_push.yml Outdated Show resolved Hide resolved
.github/test_on_push.yml Outdated Show resolved Hide resolved
.github/test_on_push.yml Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
tests/test_project_generation.py Outdated Show resolved Hide resolved
@agriyakhetarpal
Copy link
Member

P.S. I'm sorry for the late review! I have been a bit out of touch, plus I noticed that Saransh and Arjun had already put in their reviews. I hope this is helpful.

Co-authored-by: Agriya Khetarpal <[email protected]>
@arjxn-py arjxn-py mentioned this pull request Jun 12, 2024
@santacodes
Copy link
Collaborator Author

@arjxn-py can you please review my last commit, I actually modified those test cases completely because I think they're better in terms of assertions and we don't have to play with paths as pytest-cookies mostly takes care of it.
So one of the test cases tests template generation with default contexts from cookiecutter.json while the other tests template generation with a custom context passed into the bake() function. If you feel anything is wrong feel free to point it out so that I could revert it back if needed.

arjxn-py
arjxn-py previously approved these changes Jun 13, 2024
Copy link
Member

@arjxn-py arjxn-py left a comment

Choose a reason for hiding this comment

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

Thanks @santacodes, the tests look perfect to me now.

@arjxn-py
Copy link
Member

Before we merge, I'd just want to confirm if we can safely resolve the conversation about uv caching plus can you maybe link the CI run on the latest commit from your fork here?

Copy link
Member

@agriyakhetarpal agriyakhetarpal left a comment

Choose a reason for hiding this comment

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

This looks good to me as long as the tests pass – resolved the previous conversation. Thanks for adding this, @santacodes!

xrefs for uv's caching to look at later: astral-sh/uv#2231, yezz123/setup-uv#25, actions/setup-python#822, actions/setup-python#818

.github/test_on_push.yml Outdated Show resolved Hide resolved
Co-authored-by: Agriya Khetarpal <[email protected]>
@agriyakhetarpal
Copy link
Member

(I think we should disable the setting that dismisses reviews on further commits)

@arjxn-py
Copy link
Member

arjxn-py commented Jun 13, 2024

I'm merging this as @santacodes needs to start working on entrypoints as well.
Although everything looks good but in case something fails we can look into it later as well

@arjxn-py
Copy link
Member

(I think we should disable the setting that dismisses reviews on further commits)

Yes, I was going to mention that reviews were getting dismissed automatically

@arjxn-py arjxn-py merged commit dca6946 into pybamm-team:main Jun 13, 2024
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