Remove Crypto import and update tests #28

jeanconn · 2023-12-29T19:50:12Z

Description

Remove Crypto import and update tests.

Crypto isn't available for Python 3.11 and we weren't really using it anyway. A more fundamental issue is that it looks like netrc.netrc is now returning empty string instead of None if a value isn't in the netrc, so this isn't 100% back-compatible behavior (and the test isn't back-compatible).

Fixes issue like

FAILED ska_ftp/tests/test_basic.py::test_roundtrip - ModuleNotFoundError: No module named 'Crypto'

on Python 3.11 test build. May have more warnings again on Python 3.10 build but we catch those with pytest.ini anyway.

Interface impacts

test_parse_netrc has been updated for netrc version >= 3.10 which returns empty string instead of None and is not compatible with current ska3-flight.

Testing

Unit tests

Mac with ska3-flight-2024.0rc7 and ska3-masters

(ska3-flight-2024.0rc7) flame:ska_ftp jean$ pytest
=============================================================================== test session starts ===============================================================================
platform darwin -- Python 3.11.7, pytest-7.4.4, pluggy-1.3.0
rootdir: /Users/jean/git
configfile: pytest.ini
plugins: timeout-2.2.0, anyio-4.2.0
collected 7 items                                                                                                                                                                 

ska_ftp/tests/test_basic.py .......                                                                                                                                         [100%]

================================================================================ warnings summary =================================================================================
../../miniconda3/envs/ska3-flight-2024.0rc7/lib/python3.11/site-packages/ska_helpers/version.py:25
  /Users/jean/miniconda3/envs/ska3-flight-2024.0rc7/lib/python3.11/site-packages/ska_helpers/version.py:25: DeprecationWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html
    from pkg_resources import DistributionNotFound, get_distribution

../../miniconda3/envs/ska3-flight-2024.0rc7/lib/python3.11/site-packages/pkg_resources/__init__.py:2868
  /Users/jean/miniconda3/envs/ska3-flight-2024.0rc7/lib/python3.11/site-packages/pkg_resources/__init__.py:2868: DeprecationWarning: Deprecated call to `pkg_resources.declare_namespace('sphinxcontrib')`.
  Implementing implicit namespace packages (as specified in PEP 420) is preferred to `pkg_resources.declare_namespace`. See https://setuptools.pypa.io/en/latest/references/keywords.html#keyword-namespace-packages
    declare_namespace(pkg)

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
========================================================================== 7 passed, 2 warnings in 4.91s

(ska3) flame:ska_ftp jean$ git rev-parse HEAD
10d5a14abe9b464c38f9211ee68b32aaa8c31bb2
(ska3) flame:ska_ftp jean$ pytest
=============================================================================== test session starts ===============================================================================
platform darwin -- Python 3.10.8, pytest-7.2.1, pluggy-1.0.0
rootdir: /Users/jean/git, configfile: pytest.ini
plugins: timeout-2.1.0, anyio-3.6.2
collected 7 items                                                                                                                                                                 

ska_ftp/tests/test_basic.py .......                                                                                                                                         [100%]

================================================================================ 7 passed in 3.34s

Independent check of unit tests by [REVIEWER NAME]

[PLATFORM]:

Functional tests

No functional testing.

jeanconn · 2023-12-29T19:53:33Z

ska_ftp/tests/test_basic.py

@@ -90,7 +89,7 @@ def test_delete_when_ftp_session_already_gone(capfd):
 def test_parse_netrc():
    cwd = os.path.dirname(__file__)
    netrc = ska_ftp.parse_netrc(os.path.join(cwd, 'netrc'))
-    assert netrc == {'test1': {'account': None,
+    assert netrc == {'test1': {'account': '',


Not sure if this should be updated to have different values for different versions of netrc . I think what we're seeing lines up with https://docs.python.org/3/library/netrc.html "changed in version 3.10" .

I haven't looked in detail, but my first reaction is that we should try to maintain API compatibility in ska_ftp, if possible. If it is a huge pain then we can reconsider.

So would you want to code around the change in netrc.netrc to introduce api back compatibility for ska_ftp.parse_netrc?

Thinking about this some more, we are unlikely to see this cause much of problem in the wild. Basically our users are going to either have a complete netrc entry or not all.

Indeed, I couldn't think of a use case where we would be asking the netrc file for credentials when the user or password (credential pair) is incomplete.

I would prefer to have this test compare to two possible reference results, one with None and the other with ''. Either that, or do something to figure out which netrc version is present, and then use the right reference reult.

That way we can merge now instead of juggle that together with the speedy transition.

taldcroft · 2024-01-05T15:48:12Z

ska_ftp/tests/test_basic.py

 LUCKY = 'lucky.cfa.harvard.edu'


-def roundtrip(FtpClass, logger=None):
+def roundtrip(NETRC, FtpClass, logger=None):


Don't use an all-caps here, especially since it masks a global value (fixture) making things super confusing.

Optionally also change FtpClass to ftp_cls to comply with modern naming standards.

I agree about not using all-caps here.

taldcroft

Approving this with a lien on that one all-caps fix in the test. You can merge once that is fixed.

javierggt

Changes requested in the comments:

not to use NETRC in all-aps in function argument
make it so the test passes in the current setup so it is not an extra thing to consider during the speedy transition

javierggt · 2024-01-18T16:31:44Z

ska_ftp/tests/test_basic.py

 LUCKY = 'lucky.cfa.harvard.edu'


-def roundtrip(FtpClass, logger=None):
+def roundtrip(NETRC, FtpClass, logger=None):


I agree about not using all-caps here.

javierggt · 2024-01-18T16:33:59Z

ska_ftp/tests/test_basic.py

@@ -90,7 +89,7 @@ def test_delete_when_ftp_session_already_gone(capfd):
 def test_parse_netrc():
    cwd = os.path.dirname(__file__)
    netrc = ska_ftp.parse_netrc(os.path.join(cwd, 'netrc'))
-    assert netrc == {'test1': {'account': None,
+    assert netrc == {'test1': {'account': '',


I would prefer to have this test compare to two possible reference results, one with None and the other with ''. Either that, or do something to figure out which netrc version is present, and then use the right reference reult.

That way we can merge now instead of juggle that together with the speedy transition.

jeanconn · 2024-01-18T17:07:58Z

Regarding "make it so the test passes in the current setup so it is not an extra thing to consider during the speedy transition" -- we can make that one-test pass both ways if you like. If we remove the Crypto import and warnings filter here I assume you'd need an update to the global pytest ini to pass ska_testr warnings checking for the Python 3.10 case.

javierggt · 2024-01-18T17:10:49Z

and if we were to make another 2023.* release, we might have to add the ignore to the ska environment in ska_helpers, but we would be done with this.

jeanconn · 2024-01-18T18:23:10Z

OK, I renamed the fixture to parsed_netrc and updated that comparison test to work with either '' or None for an empty account value. We could also remove the test with an empty account as it seems not relevant to our use cases as noted.

javierggt · 2024-01-18T19:14:12Z

Unfortunately the naming issue is still there. The main point was that the fixture should have a different name than the argument in def roundtrip, so the argument was shadowing the global name, and it makes it harder to follow the logic, because you see the name and can't figure out what is what.

You had named the fixture in all-caps, and that is arguably ok because it is a global, so Tom's request was to change the all-caps only in the roundtrip function (he might have also disliked it in the fixture, since fixtures are not all-cap either).

Now they are both lower-case, but they should be different.

Sorry.

jeanconn · 2024-01-18T19:25:10Z

Oh OK, I'll just rename it in roundtrip(). And yes, I considered that the fixture really wasn't that different from the global that I replaced it with when I left it named NETRC but missed the point that the local test function roundtrip() doesn't need access to the fixture because it is supplied directly by the test and the naming gets a bit confusing there. Granted they all get basically the same thing and I didn't fix some of the issues of scope that existed before I started here.

I did not come across good advice for fixture naming in my limited googling.

jeanconn · 2024-01-18T19:28:58Z

If the preference is that the fixture is named NETRC I'll go back to that -- I don't feel strongly here about the naming I just really didn't like the global NETRC.

jeanconn · 2024-01-18T19:41:06Z

Maybe with this netrc fixture it would make more sense to use the usefixtures decorator.

jeanconn · 2024-01-22T20:59:59Z

OK, per our morning conversation I left the pytest fixtures implemented as arguments, renamed FtpClass to ftp_cls, and reran the tests on ska3-masters and the ska3 speedy candidate. I'll go ahead and merge this.

jeanconn · 2024-01-22T21:00:29Z

Oh, I forgot I'll need a new review status from Javier.

jeanconn added 3 commits December 29, 2023 14:45

Remove Crypto import to catch paramiko warnings

e4d82c6

Update test reference data for new netrc parsing

1d264df

Update tests to use NETRC fixture

1e9c4a6

jeanconn force-pushed the no-pycrypto branch from ca17030 to 1e9c4a6 Compare December 29, 2023 19:51

jeanconn commented Dec 29, 2023

View reviewed changes

jeanconn marked this pull request as ready for review December 29, 2023 19:54

jeanconn requested review from taldcroft and javierggt December 29, 2023 19:54

jeanconn added the speedy label Dec 30, 2023

taldcroft reviewed Jan 5, 2024

View reviewed changes

taldcroft approved these changes Jan 5, 2024

View reviewed changes

javierggt requested changes Jan 18, 2024

View reviewed changes

Rename fixture, update test, fix formatting

2990cca

Rename netrc arg in local roundtrip test func

8e37079

Rename FtpClass flt_cls

10d5a14

jeanconn requested a review from javierggt January 22, 2024 21:00

javierggt approved these changes Jan 23, 2024

View reviewed changes

jeanconn merged commit 2049021 into master Jan 23, 2024

javierggt mentioned this pull request Feb 13, 2024

Update sot/ska_ftp to 4.0.1 sot/skare3#1258

Closed

This was referenced Mar 6, 2024

2024.1 sot/skare3#1282

Merged

2024.2 sot/skare3#1287

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Crypto import and update tests #28

Remove Crypto import and update tests #28

jeanconn commented Dec 29, 2023 •

edited

Loading

jeanconn Dec 29, 2023

taldcroft Jan 2, 2024

jeanconn Jan 2, 2024

taldcroft Jan 3, 2024

jeanconn Jan 3, 2024

javierggt Jan 18, 2024

taldcroft Jan 5, 2024

javierggt Jan 18, 2024

taldcroft left a comment

javierggt left a comment

javierggt Jan 18, 2024

javierggt Jan 18, 2024

jeanconn commented Jan 18, 2024

javierggt commented Jan 18, 2024

jeanconn commented Jan 18, 2024

javierggt commented Jan 18, 2024 •

edited

Loading

jeanconn commented Jan 18, 2024

jeanconn commented Jan 18, 2024

jeanconn commented Jan 18, 2024

jeanconn commented Jan 22, 2024

jeanconn commented Jan 22, 2024

Remove Crypto import and update tests #28

Remove Crypto import and update tests #28

Conversation

jeanconn commented Dec 29, 2023 • edited Loading

Description

Interface impacts

Testing

Unit tests

Functional tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taldcroft left a comment

Choose a reason for hiding this comment

javierggt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanconn commented Jan 18, 2024

javierggt commented Jan 18, 2024

jeanconn commented Jan 18, 2024

javierggt commented Jan 18, 2024 • edited Loading

jeanconn commented Jan 18, 2024

jeanconn commented Jan 18, 2024

jeanconn commented Jan 18, 2024

jeanconn commented Jan 22, 2024

jeanconn commented Jan 22, 2024

jeanconn commented Dec 29, 2023 •

edited

Loading

javierggt commented Jan 18, 2024 •

edited

Loading