search

pybamm-team / pybamm-cookie

A copier template for battery modeling projects using PyBaMM
BSD 3-Clause "New" or "Revised" License
12 stars 3 forks source link

Added tests for template generation #9

Closed santacodes closed 4 months ago

santacodes commented 5 months ago

Description

santacodes commented 5 months ago

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.

Saransh-cpp commented 5 months ago

TODO:

santacodes commented 5 months ago

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 commented 5 months ago

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

arjxn-py commented 5 months ago

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

agriyakhetarpal commented 5 months ago

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.

santacodes commented 4 months ago

@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 commented 4 months ago

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?

agriyakhetarpal commented 4 months ago

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

arjxn-py commented 4 months ago

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 commented 4 months ago

(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