Add tests for exception raise in PSA (closes #4036) #4037

xhgchen · 2023-02-22T00:46:18Z

Closes Add tests for get_num_atoms exception raise in PSA (per issue #597) #4036
Towards Tests should make MDAnalysis raise exceptions #597
Checks that get_num_atoms in PSAnalysis and Path raises a ValueError and displays "No path data"

github-actions

Hello there first time contributor! Welcome to the MDAnalysis community! We ask that all contributors abide by our Code of Conduct and that first time contributors introduce themselves on the developer mailing list so we can get to know you. You can learn more about participating here. Please also add yourself to package/AUTHORS as part of this PR.

codecov · 2023-02-22T01:07:29Z

Codecov Report

Patch coverage has no change and project coverage change: +0.02 🎉

Comparison is base (55a5abd) 93.54% compared to head (8b10e29) 93.56%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4037      +/-   ##
===========================================
+ Coverage    93.54%   93.56%   +0.02%     
===========================================
  Files          191      191              
  Lines        25065    25065              
  Branches      4042     4042              
===========================================
+ Hits         23446    23452       +6     
+ Misses        1099     1093       -6     
  Partials       520      520

Impacted Files	Coverage Δ
package/MDAnalysis/analysis/psa.py	`85.38% <0.00%> (+1.16%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

hmacdope · 2023-02-22T21:47:02Z

testsuite/MDAnalysisTests/analysis/test_psa.py

+    run Path.to_path()"""
+    match_exp = "No path data"
+    with pytest.raises(ValueError, match=match_exp):
+        universe1 = mda.Universe(PSF, DCD)


If you are going to use these Universes more than once probably best to make them a fixture

Done! Thanks for the feedback! I also reorganized my tests to go under class TestPSAExceptions and added 2 new tests for psa.get_num_paths and psa.get_paths as I realized those functions needed similar testing. That should cover all the "no path data" type exceptions in the module.

hmacdope · 2023-02-26T08:32:25Z

testsuite/MDAnalysisTests/analysis/test_psa.py

 class TestPSAExceptions(object):
    '''Tests for exceptions that should be raised
    or caught by code in the psa module.'''

+    @pytest.fixture()


You can probably collapse these tests into one test where you call each method sequentially, would save a bit of space.

Great, I was actually wondering whether that was appropriate too but I was hesitant as I read someone mention on Stack Overflow that they preferred testing them individually. I left the fixture there because I think it may be applicable for future tests, but please let me know if it would be better to remove it.

hmacdope · 2023-02-28T21:49:11Z

testsuite/MDAnalysisTests/analysis/test_psa.py

+        # Cases where user did not run PSAnalysis.generate_paths()
+        with pytest.raises(ValueError, match=match_exp):
+            psa.get_num_atoms()
+


Can you add a quick comment for last two cases?

For sure! I finished adding new comments and also tried to make the comments more clear. Changes should be final now, but please let me know if there is anything else that can be done. I messed up the commit history while trying to rebase and resolve conflicts but I managed to fix it with a hard reset. Definitely learned a lot while troubleshooting that!

Edit: Codecov is acting strangely with the checks; did something happen?

hmacdope · 2023-03-01T19:17:13Z

@xhgchen the CI is being flaky, I will re-run it.

xhgchen · 2023-03-02T02:47:58Z

@hmacdope I pushed just now to resolve conflicts but did not make any other changes. Hopefully the checks run better this time!

hmacdope

Thanks @xhgchen looks great.
Would another @MDAnalysis/coredevs or @MDAnalysis/gsoc-mentors be able to take a quick look?

ojeda-e

Thanks @xhgchen!
The CI for the darker_lint is failing (See here). Just a quick update to the format and it's ready to go.

Pinning @hmacdope in case this is not the reason why the CI was failing before.

IAlibay · 2023-03-04T17:58:43Z

Thanks @xhgchen!
The CI for the darker_lint is failing (See here). Just a quick update to the format and it's ready to go.

Pinning @hmacdope in case this is not the reason why the CI was failing before.

Just as an FYI on this - non flake8 failure of the darker lint are fine, they should only be considered informative.

IAlibay · 2023-03-04T18:08:16Z

/azp run

azure-pipelines · 2023-03-04T18:08:26Z

Azure Pipelines successfully started running 1 pipeline(s).

xhgchen · 2023-03-04T18:37:35Z

Thank you all for taking the time to review this! I did check the darker lint failure multiple times but I left it alone after fixing the style issues in my first few commits as it did not seem to involve my code. If I was mistaken and there is anything specific I can fix, I would be happy to make those changes!

hmacdope · 2023-03-04T23:55:24Z

Yeah style things optional. :)

* Towards MDAnalysis#597 * Checks that get_num_atoms in PSAnalysis and Path raises a ValueError and displays "No path data"

* Make universe1 = mda.Universe(PSF, DCD) a fixture * Add similar tests to raise ValueErrors for get_num_paths and get_paths * Reorganize tests under class TestPSAExceptions

xhgchen · 2023-03-05T17:58:10Z

Just resolving a CHANGELOG conflict :)

orbeckst · 2023-03-09T20:46:12Z

Can we merge the PR, @hmacdope ?

hmacdope · 2023-03-09T22:00:29Z

yep all good!

github-actions bot reviewed Feb 22, 2023

View reviewed changes

xhgchen force-pushed the psa-get-num-atoms-tests branch from e75a102 to 18d63d4 Compare February 22, 2023 01:29

hmacdope requested changes Feb 22, 2023

View reviewed changes

xhgchen requested a review from hmacdope February 23, 2023 00:08

hmacdope requested changes Feb 26, 2023

View reviewed changes

xhgchen requested a review from hmacdope February 26, 2023 22:07

hmacdope requested changes Feb 28, 2023

View reviewed changes

xhgchen force-pushed the psa-get-num-atoms-tests branch from cbeffab to 7c3de5b Compare March 1, 2023 05:33

xhgchen requested a review from hmacdope March 1, 2023 05:55

orbeckst assigned hmacdope Mar 1, 2023

xhgchen force-pushed the psa-get-num-atoms-tests branch from 7c3de5b to 50fbe86 Compare March 2, 2023 02:44

xhgchen force-pushed the psa-get-num-atoms-tests branch from 50fbe86 to 8d9a012 Compare March 2, 2023 17:17

hmacdope approved these changes Mar 4, 2023

View reviewed changes

ojeda-e reviewed Mar 4, 2023

View reviewed changes

hmacdope requested a review from ojeda-e March 5, 2023 02:24

xhgchen added 7 commits March 5, 2023 10:51

Add tests for exception raise in PSA (per issue MDAnalysis#597)

00cfac8

* Towards MDAnalysis#597 * Checks that get_num_atoms in PSAnalysis and Path raises a ValueError and displays "No path data"

Fix linting issues and add to AUTHORS

1248c09

Fix comments for clarity

58fb04c

Make fixture for Universe and add paths tests

e06f21e

* Make universe1 = mda.Universe(PSF, DCD) a fixture * Add similar tests to raise ValueErrors for get_num_paths and get_paths * Reorganize tests under class TestPSAExceptions

Remove trailing whitespace

c369225

Collapse tests into one test

49bc5d9

Add and revise comments for clarity

db83998

Update CHANGELOG

8b10e29

xhgchen force-pushed the psa-get-num-atoms-tests branch from 8d9a012 to 8b10e29 Compare March 5, 2023 17:56

hmacdope merged commit ac96a65 into MDAnalysis:develop Mar 9, 2023

xhgchen deleted the psa-get-num-atoms-tests branch March 10, 2023 01:01

IAlibay added the maintainability label Sep 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for exception raise in PSA (closes #4036) #4037

Add tests for exception raise in PSA (closes #4036) #4037

xhgchen commented Feb 22, 2023 •

edited

Loading

github-actions bot left a comment

codecov bot commented Feb 22, 2023 •

edited

Loading

hmacdope Feb 22, 2023

xhgchen Feb 22, 2023 •

edited

Loading

hmacdope Feb 26, 2023

xhgchen Feb 26, 2023

hmacdope Feb 28, 2023

xhgchen Mar 1, 2023 •

edited

Loading

hmacdope commented Mar 1, 2023

xhgchen commented Mar 2, 2023

hmacdope left a comment

ojeda-e left a comment

IAlibay commented Mar 4, 2023

IAlibay commented Mar 4, 2023

azure-pipelines bot commented Mar 4, 2023

xhgchen commented Mar 4, 2023

hmacdope commented Mar 4, 2023

xhgchen commented Mar 5, 2023

orbeckst commented Mar 9, 2023

hmacdope commented Mar 9, 2023

Add tests for exception raise in PSA (closes #4036) #4037

Add tests for exception raise in PSA (closes #4036) #4037

Conversation

xhgchen commented Feb 22, 2023 • edited Loading

github-actions bot left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 22, 2023 • edited Loading

Codecov Report

hmacdope Feb 22, 2023

Choose a reason for hiding this comment

xhgchen Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

hmacdope Feb 26, 2023

Choose a reason for hiding this comment

xhgchen Feb 26, 2023

Choose a reason for hiding this comment

hmacdope Feb 28, 2023

Choose a reason for hiding this comment

xhgchen Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

hmacdope commented Mar 1, 2023

xhgchen commented Mar 2, 2023

hmacdope left a comment

Choose a reason for hiding this comment

ojeda-e left a comment

Choose a reason for hiding this comment

IAlibay commented Mar 4, 2023

IAlibay commented Mar 4, 2023

azure-pipelines bot commented Mar 4, 2023

xhgchen commented Mar 4, 2023

hmacdope commented Mar 4, 2023

xhgchen commented Mar 5, 2023

orbeckst commented Mar 9, 2023

hmacdope commented Mar 9, 2023

xhgchen commented Feb 22, 2023 •

edited

Loading

codecov bot commented Feb 22, 2023 •

edited

Loading

xhgchen Feb 22, 2023 •

edited

Loading

xhgchen Mar 1, 2023 •

edited

Loading