Skip to content

Conversation

@andrewlee94
Copy link
Contributor

@andrewlee94 andrewlee94 commented Jun 21, 2024

Fixes None

Summary/Motivation:

Run times for our non-integration tests have been creeping up again, so this PR moves a lot of slow tests (>10s) to integration. This will likely impact our apparent test coverage, as this is calculated based on the unit and component flags only, but cuts the run time for these by nearly 30% on my local machine.

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.

@andrewlee94 andrewlee94 self-assigned this Jun 21, 2024
@andrewlee94 andrewlee94 added Priority:Normal Normal Priority Issue or PR testing Issues dealing with testing of code models_extra Issues related to models in models_extra folder. labels Jun 21, 2024
Copy link
Contributor

@dallan-keylogic dallan-keylogic left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 3548 to 3550
def test_general_hx1d_initializer(self, model):
initializer = HX1DInitializer()
initializer.initialize(model.fs.unit)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder how this initialization time is going to be affected when we move away from block triangularization in #1436 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a good point, but I suspect it won't have a huge effect in this case. I believe there is another equivalent test for a different configuration that runs fast enough, so I suspect this one is caused more by stiffness in the model than the property initialization. Still, would be worth waiting for #1436 to check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update: after #1436 merged, run time for this test is still ~16 seconds, so I'll leave it in integration for now.

@andrewlee94 andrewlee94 enabled auto-merge (squash) June 26, 2024 19:18
@codecov-commenter
Copy link

codecov-commenter commented Jun 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.35%. Comparing base (2e58de6) to head (9df8966).
Report is 24 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1442      +/-   ##
==========================================
- Coverage   77.74%   76.35%   -1.39%     
==========================================
  Files         394      394              
  Lines       64953    64953              
  Branches    14404    14404              
==========================================
- Hits        50497    49598     -899     
- Misses      11875    12797     +922     
+ Partials     2581     2558      -23     

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

@andrewlee94 andrewlee94 disabled auto-merge June 26, 2024 19:58
@andrewlee94 andrewlee94 merged commit 10b42e4 into IDAES:main Jun 26, 2024
@andrewlee94 andrewlee94 deleted the long_test_audit branch June 26, 2024 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

models_extra Issues related to models in models_extra folder. Priority:Normal Normal Priority Issue or PR testing Issues dealing with testing of code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants