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

Add unit tests for the Jinja2 filters #37

Closed
cwhitlock-NOAA opened this issue Oct 25, 2024 · 3 comments · Fixed by #33
Closed

Add unit tests for the Jinja2 filters #37

cwhitlock-NOAA opened this issue Oct 25, 2024 · 3 comments · Fixed by #33
Assignees
Labels
enhancement New feature or request

Comments

@cwhitlock-NOAA
Copy link
Contributor

Ian makes a very good case for why we want one set of unit tests in fre-workflows. The Jinja2 filters are...

  • doing a function that gets exercised every time that we run an experiment
  • touch-it-and-break-it not in the sense that the code is fragile, but in the sense that you're likely to get unexpected consequences from a simple edit because they do so much
  • written in python, and thus amenable to a pytest freamework
  • do something readily testable (do we get this output when we put in this input?)

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.

@ceblanton
Copy link
Contributor

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.

@cwhitlock-NOAA
Copy link
Contributor Author

cwhitlock-NOAA commented Oct 25, 2024 via email

@ilaflott
Copy link
Member

ilaflott commented Oct 26, 2024

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.

@ilaflott ilaflott linked a pull request Oct 29, 2024 that will close this issue
@ilaflott ilaflott added the enhancement New feature or request label Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants