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

Boron removal unit test harness #1415

Merged
merged 11 commits into from
Jun 9, 2024

Conversation

savannahsakhai
Copy link
Contributor

Fixes/Resolves:

#1302

Changes proposed in this PR:

  • Applying the unit test harness for the boron removal unit

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

@savannahsakhai savannahsakhai self-assigned this May 30, 2024
@ksbeattie ksbeattie added the Priority:High High Priority Issue or PR label May 30, 2024
Copy link

codecov bot commented May 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.00%. Comparing base (82f5fcb) to head (9ec6d83).
Report is 47 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1415      +/-   ##
==========================================
- Coverage   94.01%   94.00%   -0.02%     
==========================================
  Files         335      335              
  Lines       35572    35572              
==========================================
- Hits        33444    33440       -4     
- Misses       2128     2132       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@savannahsakhai savannahsakhai marked this pull request as ready for review June 3, 2024 17:48
Comment on lines -599 to -605
# Start test class
class TestBoronRemoval_GenPropPack:
@pytest.fixture(scope="class")
def gen_boron_removal_model(self):
m = ConcreteModel()
m.fs = FlowsheetBlock(dynamic=False)

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless I missed it, this gen prop pack test is missing from the update version. Is there a reason this test should be left out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had left it out initially because it had seemed redundant to test the unit model with more than 1 property model (and using a property model outside of watertap). However, if this is an important aspect for testing, I can add on these tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it can be left out. I just wanted to make sure you had deleted it intentionally rather than accidentally leaving it out.

Copy link
Contributor

@MarcusHolly MarcusHolly left a comment

Choose a reason for hiding this comment

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

LGTM

@bknueven bknueven enabled auto-merge (squash) June 9, 2024 21:44
@bknueven bknueven merged commit 617fe3a into watertap-org:main Jun 9, 2024
24 checks passed
@MarcusHolly MarcusHolly mentioned this pull request Jun 20, 2024
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:High High Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants