Skip to content

testing dedup, stdsuite upgrades #2632

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
merged 1 commit into from
Jul 14, 2022
Merged

testing dedup, stdsuite upgrades #2632

merged 1 commit into from
Jul 14, 2022

Conversation

loriab
Copy link
Member

@loriab loriab commented Jul 13, 2022 •
edited
Loading

Description

Todos

  • Nowadays, ctest (quick) followed by pytest (quick) runs a lot of duplicate tests (unless the latter uses "quick and api"). Let's not do that. This switches Azure to ctest(smoke) followed by pytest (quick).
    • before:
      • W: 1h30m
      • L: 41-57m
    • after:
      • W: 1h5m
      • L: 34-38m
  • In working on dfocc, I found some stdsuite updates that hadn't been ported from qcng to psi4 and some general improvements.
    • better check expected vs intended modules. (can catch if the default compute module for a method+circs changes.)
    • test_*_default in stdsuite changed meaning slightly. now PASSED means get final method answer correctly and XFAIL means known NYI. formerly, inputs that got the final method answer correctly but were not able to supply some submethod (like same-spin mp2 for ccsd) also got the XFAIL result.
    • have a mini ref file so can add reference data w/o waiting for a new qcengine release
    • regex and human-directed error messages to simplify and consolidate NYI methods testing

Checklist

  • Tests added for any new features
  • full stdsuite runs

Status

  • Ready for review
  • Ready for merge

@loriab loriab changed the title WIP: testing dedup, stdsuite upgrades testing dedup, stdsuite upgrades Jul 13, 2022
@loriab loriab added the testing label Jul 13, 2022
@loriab loriab added this to the Psi4 1.7 milestone Jul 13, 2022
JonathonMisiewicz
JonathonMisiewicz reviewed Jul 14, 2022
View reviewed changes
tests/pytests/standard_suite_ref_local.py
from qcengine.programs.tests.standard_suite_ref import answer_hash, _std_suite, _std_generics


_std_suite_psi4_extension = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Please include a comment explaining how to add items to this file and why.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's key/value pairs to extend data of dict keys in the same format as the qcng file at the top of this file, https://github.com/MolSSI/QCEngine/blob/master/qcengine/programs/tests/standard_suite_ref.py#L447. I'm not sure that extra comments help more than the unavoidable reading/searching.

I agree stdsuite as a whole could use some procedural documentation (what repos to open and edit, what options to have engaged, what sub-checks to disengage at first, what tests to set up for a mtd) that I'm rather at a loss to know where to start. This part, though, is navigable by search-and-extend.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're the only developer who knows how the standard suite machinery works. Can you at least document somewhere when to add an entry to this file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I can add the below in a future PR.

# in-repo extensions for _std_suite above
# * ideally empty. PR to QCEngine ASAP and empty this after QCEngine release.
_std_suite_psi4_extension = [

JonathonMisiewicz
JonathonMisiewicz approved these changes Jul 14, 2022
View reviewed changes
Copy link
Contributor

@JonathonMisiewicz JonathonMisiewicz left a comment

Choose a reason for hiding this comment

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

I do request a docs improvement prior to merging this in, but otherwise LGTM.

@loriab loriab merged commit ad20635 into psi4:master Jul 14, 2022
@loriab loriab deleted the stdsuitedd branch July 14, 2022 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JonathonMisiewicz JonathonMisiewicz JonathonMisiewicz approved these changes

@maxscheurer maxscheurer maxscheurer approved these changes

@zachglick zachglick zachglick approved these changes

Assignees
No one assigned
Labels
testing
Projects
None yet
Milestone
Psi4 1.7
Development

Successfully merging this pull request may close these issues.
4 participants
@loriab @maxscheurer @zachglick @JonathonMisiewicz