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

Update Testing Docs #149

Merged
merged 8 commits into from
Mar 1, 2024
Merged

Update Testing Docs #149

merged 8 commits into from
Mar 1, 2024

Conversation

jwaldrop107
Copy link
Member

Description
This PR fills in some of the missing documentation on testing.

@jwaldrop107 jwaldrop107 self-assigned this Feb 29, 2024
@jwaldrop107 jwaldrop107 requested a review from ryanmrichard March 1, 2024 16:06
Copy link
Member

@ryanmrichard ryanmrichard left a comment

Choose a reason for hiding this comment

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

What you have is fine, but I'm not sure it's what we want long term. Happy to discuss.

docs/source/testing/integration.rst Show resolved Hide resolved
docs/source/testing/integration.rst Outdated Show resolved Hide resolved
docs/source/testing/integration.rst Outdated Show resolved Hide resolved
@jwaldrop107 jwaldrop107 requested a review from ryanmrichard March 1, 2024 18:09

def setUp(self):
self.mm = ModuleManager()
nwchemex.load_modules(mm) # Also loads out SCF modules
Copy link
Member

Choose a reason for hiding this comment

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

Fine for now, but not sure this is going to work in practice. In particular, I'm not convinced it will find the SCF version we want to test vs. the release version NWX depends on. We can worry about that later though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wondered about that, as well. My expectation is that the version that is being built at the time should be found by CMake, so it shouldn't try to pull and build the version specified within NWChemEx. I think it should also take precedent over installed versions, but I'm not sure.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's going to depend on the Python path order if there's an installed version competing with the just built version. An "easy" way around this is:

import scf # Ensure import of this repo
import nwchemex

self.mm = ModuleManager()
scf.load_modules(mm)
nwchemex.load_modules(mm)

The trick would be to make sure that load_modules is a no-op if the keys already exist.

Copy link
Member

Choose a reason for hiding this comment

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

It's also worth noting this only an issue for NWChemEx authors. Contributors don't have their plugins shipped with NWChemEx.

ryanmrichard
ryanmrichard previously approved these changes Mar 1, 2024
@jwaldrop107 jwaldrop107 requested a review from yzhang-23 March 1, 2024 18:52
@jwaldrop107 jwaldrop107 enabled auto-merge March 1, 2024 18:52
docs/source/testing/integration.rst Show resolved Hide resolved
docs/source/testing/integration.rst Outdated Show resolved Hide resolved
docs/source/testing/integration.rst Show resolved Hide resolved
docs/source/testing/integration.rst Outdated Show resolved Hide resolved
docs/source/testing/integration.rst Outdated Show resolved Hide resolved
docs/source/testing/integration.rst Show resolved Hide resolved
docs/source/testing/unit.rst Outdated Show resolved Hide resolved
@jwaldrop107 jwaldrop107 merged commit ac6aeaa into master Mar 1, 2024
4 checks passed
@jwaldrop107 jwaldrop107 deleted the update_testing_docs branch March 1, 2024 23:27
@jwaldrop107
Copy link
Member Author

🚀 [bumpr] Bumped! New version:v0.3.44 Changes:v0.3.43...v0.3.44

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.

3 participants