Closed cwhitlock-NOAA closed 4 weeks ago
Nice one! Absolutely agree that these Jinja2 python method filters are essential and fragile and so far totally untested.
The pytests for these could live here, but could they be imports of fre-cli? We could perhaps keep the python methods in fre-cli and import them here. Would that be any better? The fre-cli pytest framework is strong and established, and one could be established here too though.
I'd need to look at the code to be sure, but I thought that the Jinja2 filters were present in fre-workflows and only used in fre-workflows. If they're only used in fre-workflows, I would argue that it makes sense to keep both the base code (Jinja2 filter functions) and tests in the same repo that they are called in.
But, the nice thing about pytest is that both the structure of the tests and the framework from which they are called are fairly simple - I could start writing the tests before figuring out whether the tests will eventually live in fre-cli or fre-workflows. If we decide to move the Jinja2 filter functions, moving the tests would be pretty straightforward.
On Fri, Oct 25, 2024 at 1:37 PM Chris Blanton @.***> wrote:
Nice one! Absolutely agree that these Jinja2 python method filters are essential and fragile and so far totally untested.
The pytests for these could live here, but could they be imports of fre-cli? We could perhaps keep the python methods in fre-cli and import them here. Would that be any better? The fre-cli pytest framework is strong and established, and one could be established here too though.
— Reply to this email directly, view it on GitHub https://github.com/NOAA-GFDL/fre-workflows/issues/37#issuecomment-2438420248, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC362WQTIBIONDEUGHDGPDLZ5J6T3AVCNFSM6AAAAABQTW5JJCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMZYGQZDAMRUHA . You are receiving this because you were assigned.Message ID: @.***>
-- Carolyn Whitlock
I'd need to look at the code to be sure, but I thought that the Jinja2
filters were present in fre-workflows and only used in fre-workflows. If
they're only used in fre-workflows, I would argue that it makes sense to
keep both the base code (Jinja2 filter functions) and tests in the same
repo that they are called in.
But, the nice thing about pytest is that both the structure of the tests
and the framework from which they are called are fairly simple - I could
start writing the tests before figuring out whether the tests will
eventually live in fre-cli or fre-workflows. If we decide to move the
Jinja2 filter functions, moving the tests would be pretty straightforward.
+1 from me- couldn't make it more pragmatic or efficient an approach if i tried.
Ian makes a very good case for why we want one set of unit tests in fre-workflows. The Jinja2 filters are...
I'd mark this as lower priority than the integration tests because we aren't editing the filters yet, but I can work on a simple set of tests for these too. It's also going to help to have something more complex than a "hello world" but simpler than an integration test for working with the CI runners.