Skip to content

resolve matopt test errors #1632

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.

Sign up for GitHub

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

Jump to bottom
Merged
adam-a-a merged 4 commits into from
Jul 1, 2025
Merged

resolve matopt test errors #1632

adam-a-a merged 4 commits into from
Jul 1, 2025

Conversation

@adam-a-a
Copy link
Contributor

@adam-a-a adam-a-a commented Jun 4, 2025 •
edited
Loading

Fixes

Summary/Motivation:

Fixes matopt test errors now occurring on all PRs, presumably due to some change in pytest that is more stringent about using fixtures

NOTE: merge this pr asap to resolve test errors on all other PRs

Changes proposed in this PR:

  • use fixtures in matopt tests appropriately

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.

@dallan-keylogic
Copy link
Contributor

dallan-keylogic commented Jun 4, 2025

This PR turns a lot of tests into "fixtures", but most of these fixtures don't appear to be called by actual tests. If the thing being tested is object creation, just remove the return statement from the tests. For the few fixtures that are called by other tests, you should remove "test" from the function name.

@codecov
Copy link

codecov bot commented Jun 4, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.05%. Comparing base (6a588c7) to head (948f603).
⚠️ Report is 9 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1632      +/-   ##
==========================================
- Coverage   77.06%   77.05%   -0.02%     
==========================================
  Files         395      395              
  Lines       63512    63512              
  Branches    10358    10358              
==========================================
- Hits        48946    48939       -7     
- Misses      12126    12134       +8     
+ Partials     2440     2439       -1

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@adam-a-a
Copy link
Contributor Author

adam-a-a commented Jun 5, 2025

This PR turns a lot of tests into "fixtures", but most of these fixtures don't appear to be called by actual tests. If the thing being tested is object creation, just remove the return statement from the tests. For the few fixtures that are called by other tests, you should remove "test" from the function name.

I'll admit that I was lazy about that once I realized what you did. I merely wanted to fix some broken tests for what seemed to be unmaintained code--to get another PR closer to being merged. If you think it's important to make the suggested changes, I can do that.

@adam-a-a
Copy link
Contributor Author

adam-a-a commented Jun 5, 2025

@dallan-keylogic added the changes

radhakrishnatg
radhakrishnatg requested changes Jun 5, 2025
View reviewed changes
Copy link
Contributor

@radhakrishnatg radhakrishnatg left a comment

Choose a reason for hiding this comment

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

Looks good to me. But I think you missed a few more fixture names. Could you please fix them? Thank you!

idaes/apps/matopt/tests/test_matopt_objects_construction.py


@pytest.fixture
def test_construct_SumNeighborSites(construct_MatOptModel):
Copy link
Contributor

@radhakrishnatg radhakrishnatg Jun 5, 2025

Choose a reason for hiding this comment

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

Please remove test_ from the fixture name. Same comment for the next five fixture names.

Copy link
Contributor Author

@adam-a-a adam-a-a Jun 10, 2025

Choose a reason for hiding this comment

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

Did this in latest changes (that I'm about to push)

idaes/apps/matopt/tests/test_matopt_objects_construction.py
canvas.addShell(lattice.getNeighbors)
canvas.setNeighborsFromFunc(lattice.getNeighbors)
confs = [[None] * len(canvas.NeighborhoodIndexes[0]) for _ in range(1)]
a0, a1 = test_construct_Atom()
Copy link
Contributor

@radhakrishnatg radhakrishnatg Jun 5, 2025

Choose a reason for hiding this comment

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

Is there a reason why these lines were removed?

Copy link
Contributor Author

@adam-a-a adam-a-a Jun 10, 2025

Choose a reason for hiding this comment

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

oversight---bringing these lines back

idaes/apps/matopt/tests/test_matopt_objects_construction.py Outdated
canvas = construct_Canvas
m = construct_MatOptModel
r = construct_GreaterThan
print(m)
Copy link
Contributor

@radhakrishnatg radhakrishnatg Jun 5, 2025

Choose a reason for hiding this comment

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

Please remove the print statement.

Copy link
Contributor Author

@adam-a-a adam-a-a Jun 10, 2025

Choose a reason for hiding this comment

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

Done

@adam-a-a adam-a-a requested a review from radhakrishnatg June 10, 2025 20:53
@adam-a-a
Copy link
Contributor Author

adam-a-a commented Jun 10, 2025

@dallan-keylogic @radhakrishnatg - I addressed your changes. Please note that without these changes, tests on all PRs will fail and we cannot merge anything else until this goes through (hence the imperfect changes in this PR).

radhakrishnatg
radhakrishnatg approved these changes Jun 10, 2025
View reviewed changes
Copy link
Contributor

@radhakrishnatg radhakrishnatg left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@adam-a-a
Copy link
Contributor Author

adam-a-a commented Jun 12, 2025

didn't go through the release notes in detail but it looks like pytest version 8.4.0 is causing these tests to fail: https://github.com/pytest-dev/pytest/releases

We can try pinning pytest to 8.3.5

@adam-a-a
Copy link
Contributor Author

adam-a-a commented Jun 13, 2025

Let's see if I handled all outliers. If somehow, many more are unveiled, I will just try pinning pytest to version 8.3.5 and post an issue. Otherwise, the PR should be ready to merge assuming no other tests fail.

@dallan-keylogic
Copy link
Contributor

dallan-keylogic commented Jun 13, 2025 

    File "/home/runner/miniconda3/envs/idaes-pse-dev/lib/python3.12/http/client.py", line 300, in _read_status
      raise RemoteDisconnected("Remote end closed connection without"
  http.client.RemoteDisconnected: Remote end closed connection without response

I wonder if this is downstream of the Google Cloud outage yesterday.

@adam-a-a adam-a-a enabled auto-merge (squash) June 16, 2025 15:30
@adam-a-a adam-a-a merged commit b7d04f7 into IDAES:main Jul 1, 2025
194 of 284 checks passed
@ksbeattie ksbeattie added the Priority:Normal Normal Priority Issue or PR label Aug 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@agarciadiego agarciadiego agarciadiego approved these changes

@radhakrishnatg radhakrishnatg radhakrishnatg approved these changes

@MarcusHolly MarcusHolly MarcusHolly approved these changes

@lbianchi-lbl lbianchi-lbl Awaiting requested review from lbianchi-lbl

@ksbeattie ksbeattie Awaiting requested review from ksbeattie ksbeattie is a code owner

@dallan-keylogic dallan-keylogic Awaiting requested review from dallan-keylogic

@bpaul4 bpaul4 Awaiting requested review from bpaul4 bpaul4 is a code owner

Assignees

No one assigned

Labels

Priority:Normal Normal Priority Issue or PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@adam-a-a @dallan-keylogic @agarciadiego @radhakrishnatg @MarcusHolly @ksbeattie