Adds test to surface plotting for closed=True and "full torus"

[daringli]

• Adds test coverage for "full torus" closed=True Surface.plot()
• Adds a warning if a non-3D matplotlib axis is passed to Surface.plot()

The test asserts that aspects of the plot has not changed. Downside: tests may break if matplotlib updates. Waiting for the CI to tell me if this is a serious problem or not. The old tests for the plots didn't actually assert anything so would not be able to catch regression problems that unless exceptions were raised.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.34%. Comparing base (f0f0ec4) to head (0810f22).
Report is 155 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #510      +/-   ##
==========================================
+ Coverage   92.25%   92.34%   +0.08%     
==========================================
  Files          82       82              
  Lines       16015    16019       +4     
==========================================
+ Hits        14775    14792      +17     
+ Misses       1240     1227      -13     

Flag	Coverage Δ
unittests	92.34% <100.00%> (+0.08%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[akaptano]

Happy to approve this once you add a test of the logger warning, which is currently tanking the code coverage percentage here. Thanks!

[daringli]

Happy to approve this once you add a test of the logger warning, which is currently tanking the code coverage percentage here. Thanks!

I didn't do it since I wasn't sure how to check for warnings from logging.Logger.
But I think I've figured it out now: unittest has a submodule unittest.mock which can be used to replace objects with mock objects that don't do anything, but logs any method call made on them. They also allow you to assert that a given method has been called, which is how it fits into the unittest framework. I use this to replace the logging.Logger.warning() method and assert that this mocked-up logging method has been called.

This way of testing is not used elsewhere in simsopt; we generally don't seem to have tests for warnings, just exceptions. Let me know if you think there is a better way to test for warning so that we do it right from the start.

[akaptano]

Happy to approve this once you add a test of the logger warning, which is currently tanking the code coverage percentage here. Thanks!

I didn't do it since I wasn't sure how to check for warnings from logging.Logger. But I think I've figured it out now: unittest has a submodule unittest.mock which can be used to replace objects with mock objects that don't do anything, but logs any method call made on them. They also allow you to assert that a given method has been called, which is how it fits into the unittest framework. I use this to replace the logging.Logger.warning() method and assert that this mocked-up logging method has been called.

This way of testing is not used elsewhere in simsopt; we generally don't seem to have tests for warnings, just exceptions. Let me know if you think there is a better way to test for warning so that we do it right from the start.

Is there a reason you can't use the default python warnings package instead of the logger warning? Warnings from this package can be easily tested.

[daringli]

Is there a reason you can't use the default python warnings package instead of the logger warning? Warnings from this package can be easily tested.

No reason except that that's seemingly not how warnings are done in the rest of SIMSOPT, except for a in a few files.

[mishapadidar]

we should probably avoid using non-public attributes

[mishapadidar]

im having a hard time understanding what this test is covering?

[mishapadidar]

@daringli can you take a look at this again?