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.

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

hiop: Corrected test name, added docstring, and changed test to new API #44765

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from
Open

hiop: Corrected test name, added docstring, and changed test to new API #44765

wants to merge 11 commits into from

Conversation

AcriusWinter
Copy link
Contributor

@AcriusWinter AcriusWinter commented Jun 18, 2024 •
edited by tldahlgren
Loading

Supersedes #35751

@spackbot-app spackbot-app bot added stand-alone-tests Stand-alone (or smoke) tests for installed packages update-package labels Jun 18, 2024
@AcriusWinter
Copy link
Contributor Author

==> [2024-06-18-13:11:34.639159] Completed testing
==> [2024-06-18-13:11:34.639335]
======================== SUMMARY: hiop-develop-onaluo4 =========================
Hiop::test .. PASSED
============================= 1 passed of 1 parts ==============================

@spackbot-app spackbot-app
Copy link

spackbot-app bot commented Jun 18, 2024

@ryandanehy can you review this PR?

This PR modifies the following package(s), for which you are listed as a maintainer:

  • hiop

tldahlgren
tldahlgren requested changes Jun 18, 2024
View reviewed changes
Copy link
Contributor

@tldahlgren tldahlgren left a comment

Choose a reason for hiding this comment

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

Nice first try. See comments.

var/spack/repos/builtin/packages/hiop/package.py Outdated Show resolved Hide resolved
var/spack/repos/builtin/packages/hiop/package.py Outdated Show resolved Hide resolved
var/spack/repos/builtin/packages/hiop/package.py Show resolved Hide resolved
var/spack/repos/builtin/packages/hiop/package.py Outdated Show resolved Hide resolved
var/spack/repos/builtin/packages/hiop/package.py Show resolved Hide resolved
@tldahlgren tldahlgren changed the title hiop: Corrected test name, added docstring, and changed test format from old to new hiop: Corrected test name, added docstring, and changed test to new API Jun 18, 2024
@tldahlgren tldahlgren self-assigned this Jun 18, 2024
@AcriusWinter
Copy link
Contributor Author

======================== SUMMARY: hiop-develop-vvcm776 =========================
Hiop::test_N1pMDsEx1_1 .. PASSED
Hiop::test_N1pMDsEx1_2 .. PASSED
Hiop::test_N1pMDsEx1_3 .. PASSED
Hiop::test_N1pMDsEx1 .. PASSED
Hiop::test_N1pMDsEx1Raja_1 .. PASSED
Hiop::test_N1pMDsEx1Raja_2 .. PASSED
Hiop::test_N1pMDsEx1Raja_3 .. PASSED
Hiop::test_N1pMdsEx1Raja .. PASSED
============================= 8 passed of 8 parts ==============================

cameronrutherford
cameronrutherford approved these changes Jun 19, 2024
View reviewed changes
Copy link
Contributor

@cameronrutherford cameronrutherford left a comment

Choose a reason for hiding this comment

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

LGTM but I defer to @tldahlgren since they are more of an expert in spack testing than I

tldahlgren
tldahlgren requested changes Jun 19, 2024
View reviewed changes
Copy link
Contributor

@tldahlgren tldahlgren left a comment

Choose a reason for hiding this comment

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

It's looking good. Just a couple of more changes remaining.

var/spack/repos/builtin/packages/hiop/package.py Outdated Show resolved Hide resolved
@cameronrutherford cameronrutherford mentioned this pull request Jun 20, 2024
Merged
tldahlgren
tldahlgren requested changes Jun 20, 2024
View reviewed changes
Copy link
Contributor

@tldahlgren tldahlgren left a comment

Choose a reason for hiding this comment

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

See comments.

var/spack/repos/builtin/packages/hiop/package.py Outdated Show resolved Hide resolved
var/spack/repos/builtin/packages/hiop/package.py Outdated Show resolved Hide resolved
var/spack/repos/builtin/packages/hiop/package.py Outdated Show resolved Hide resolved
tldahlgren
tldahlgren requested changes Jun 24, 2024
View reviewed changes
Copy link
Contributor

@tldahlgren tldahlgren left a comment

Choose a reason for hiding this comment

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

Minor tweak.

var/spack/repos/builtin/packages/hiop/package.py
)
for i, args in enumerate(options):
with test_part(
self, f"test_{exName}_{i+1}", purpose="{0} {1}".format(str(exe), " ".join(args))
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 I now regret suggesting inclusion of {exe} in the purpose as it is very messy with the entire path. Better to just output the joining of the args.

 test: test_NlpMdsEx1.exe_2: $spack/opt/spack/linux-rhel8-x86_64_v3/gcc-12.1.1/hiop-1.0.3-vecd7zfgdeksyfqdlm32n5feamcehcpi/bin/NlpMdsEx1.exe 400 100 1 -selfcheck

There's also some redundancy with the output from Spack's Executable but the documentation for the examples seems pretty sparse so can't really get something more useful there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cameronrutherford cameronrutherford cameronrutherford approved these changes

@tldahlgren tldahlgren tldahlgren requested changes

@pelesh pelesh Awaiting requested review from pelesh

Requested changes must be addressed to merge this pull request.

Assignees

@tldahlgren tldahlgren

Labels
stand-alone-tests Stand-alone (or smoke) tests for installed packages update-package
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@AcriusWinter @pelesh @cameronrutherford @tldahlgren