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

Moving some slow tests to integration #1442

Merged
merged 2 commits into from
Jun 26, 2024

Conversation

andrewlee94
Copy link
Member

@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
Member 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
Member 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 merged commit 10b42e4 into IDAES:main Jun 26, 2024
52 of 55 checks passed
@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