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 binding model submodule tests to CI #360

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

jbreue16
Copy link
Contributor

@jbreue16 jbreue16 commented Jan 16, 2025

This PR adds all binding model submodule tests to the CI, e.g. Jacobian tests for the bindings, without other modules like the unit operation.

fixes #356

TODO

  • include tests in CI
  • fix tests that fail

Fix FD pattern sign tolerance in Fruendlich LDF test

In the test settings, remove EXT_ prefix from constant binding
parameters, fixing the configuration of external binding parameter
handler
@ronald-jaepel
Copy link
Collaborator

I'm torn on adding the BindingModel tag to the CI vs adding the CI tag to the binding model tests. I think the latter would be a bit more explicit. If someone wants to run the "CI" tests locally, I don't want them to have to remember to also run the BindingModel tests. What do you think?

@jbreue16
Copy link
Contributor Author

jbreue16 commented Jan 16, 2025

Thought the exact same thing but then didnt see an easy way to parameterize that.
So lets just do it without the parameterization, i.e. every test of that kind will now be added to the CI. Which is what we want for the existing ones but maybe not anymore in the future.. and if that ever happens, we can think about the parameterization.

@ronald-jaepel
Copy link
Collaborator

ronald-jaepel commented Jan 16, 2025

I like it. Thanks for the quick change! I also think it's fine to include all binding tests by default, because they are super quick. LGTM now. Once the tests have passed again you or I can squash and merge.

@jbreue16
Copy link
Contributor Author

jbreue16 commented Jan 16, 2025

wait, this is the same error cause we had in #273
I think it didnt happen with the separate flags because this also dictates the order of execution

Ill look into it

@ronald-jaepel
Copy link
Collaborator

Could be. Thanks for looking into it! Let me know if you want to debug together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Include binding Jacobian tests in CI
2 participants