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

Remove all references to mbuild loaders so tests begin to work #862

Merged
merged 2 commits into from
Dec 18, 2024

Conversation

CalCraven
Copy link
Contributor

No description provided.

Copy link
Contributor

@chrisjonesBSU chrisjonesBSU left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks

Since mBuild, foyer and GMSO are so interconnected, I wonder if part of the CI workflow of each repo should include 1 bleeding edge test for the other 2 repos. For example, an mBuild PR runs GMSO unit tests and foyer unit tests, a GMSO PR runs mBuild and foyer unit tests, etc..

@CalCraven
Copy link
Contributor Author

Hmmm, I'll have to think about this. Ideally, we're not making many changes that should break the cross-compatibility as those are pretty significant modifications. Now that we have these stable releases, I think we should limit any of these changes to the develop branches as well. In that case, this would just add a lot of extra testing to any patch changes to the main branches. The other concern is that if either package has failing tests due to some package issue, or something else that needs breaks, then both packages will automatically fail. Which might be annoying if we're trying to add features to one package while the other is failing, say with a numpy 2.0 type situation.

@chrisjonesBSU
Copy link
Contributor

Those are good points. Also, this kind of testing is something that can just be done manually if we suspect it needs it.

Copy link

codecov bot commented Dec 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.85%. Comparing base (c8c62ae) to head (f42b66a).
Report is 9 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #862      +/-   ##
==========================================
- Coverage   94.07%   89.85%   -4.23%     
==========================================
  Files          65       65              
  Lines        6953     6976      +23     
==========================================
- Hits         6541     6268     -273     
- Misses        412      708     +296     

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

@CalCraven CalCraven merged commit 5cfa964 into mosdef-hub:main Dec 18, 2024
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants