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

cleanup: known test fault, codeql #423

Merged
merged 1 commit into from
Aug 14, 2023
Merged

Conversation

loriab
Copy link
Collaborator

@loriab loriab commented Aug 8, 2023

Description

  • there's a highly symmetric system that different versions of nwchem return a different root for. I've kept it at the one our CI uses, but others (7 tests fail #377) have hit it. so let's allow it.
  • fix a couple codeql complaints to see what that looks like.

Status

  • Code base linted
  • Ready to go

@codecov
Copy link

codecov bot commented Aug 8, 2023

Codecov Report

Merging #423 (cb02e8d) into master (5935117) will decrease coverage by 0.05%.
The diff coverage is 53.84%.

Additional details and impacted files

Copy link
Collaborator

@WardLT WardLT left a comment

Choose a reason for hiding this comment

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

LGTM.

Is the ".pop" the thing CodeQL complained about? Any idea what the problem with the one-line version is?

@loriab
Copy link
Collaborator Author

loriab commented Aug 14, 2023

Is the ".pop" the thing CodeQL complained about? Any idea what the problem with the one-line version is?

Yeah, CodeQL didn't like the unused res in res = qcng.compute(resi, program, raise_error=True, return_dict=True). And it didn't like that there were side-effects (pop) in a statement that could be optimized out (assert) in assert ret.extras.pop("psiapi_evaluated", False). Those two I at least understand, but many of CodeQL's complaints are like this https://github.com/MolSSI/QCEngine/security/code-scanning/156 where I've clearly made a copy before altering input parameters, so if anyone understands the trouble or a solution, I'd be glad to hear of it.

@loriab loriab merged commit 237ffb9 into MolSSI:master Aug 14, 2023
@loriab loriab deleted the codeqlfixes branch August 14, 2023 15:35
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