Refactoring: use tmp path fixture to mock remote and local for transport plugins #6627
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6627 +/- ##
==========================================
+ Coverage 77.51% 77.88% +0.38%
==========================================
Files 560 567 +7
Lines 41444 42149 +705
==========================================
+ Hits 32120 32825 +705
Misses 9324 9324 ☔ View full report in Codecov by Sentry. |
unkcpz
force-pushed
the
Remove-the-use-of-deprecated-tranport-methods-in-tests
branch
2 times, most recently
from
aeb255f
to
77e0074
Compare
unkcpz
force-pushed
the
Remove-the-use-of-deprecated-tranport-methods-in-tests
branch
from
58b1668
to
f19067c
Compare
unkcpz
force-pushed
the
Remove-the-use-of-deprecated-tranport-methods-in-tests
branch
from
f2c9064
to
db72976
Compare
unkcpz
force-pushed
the
Remove-the-use-of-deprecated-tranport-methods-in-tests
branch
from
56527c6
to
f9bfea9
Compare
for more information, see https://pre-commit.ci
|
When running tests, I got a lot resource not clean warnings which may indicate some resource leak: This require some investigation. |
unkcpz
changed the title
unkcpz
force-pushed
the
Remove-the-use-of-deprecated-tranport-methods-in-tests
branch
3 times, most recently
from
a0c5056
to
3c1a702
Compare
| def test_makedirs(custom_transport): | ||
| """Verify the functioning of makedirs command""" | ||
| def test_deprecate_chdir_and_getcwd(custom_transport, remote_tmp_path): | ||
| """Test to be deprecated ``chdir``/``getcwd`` methods still work.""" |
There was a problem hiding this comment.
Can we run this test only for those transport plugins that:
hasattr('chdir') or hasattr('getdir')
There was a problem hiding this comment.
But at the moment, all three transport all has these methods, no?
| """Fixture that parametrizes over all the registered implementations of the ``CommonRelaxWorkChain``.""" | ||
| plugin = TransportFactory(request.param) | ||
|
|
||
| if request.param == 'core.ssh': | ||
| kwargs = {'machine': 'localhost', 'timeout': 30, 'load_system_host_keys': True, 'key_policy': 'AutoAddPolicy'} | ||
| elif request.param == 'core.ssh_auto': | ||
| kwargs = {'machine': 'localhost'} | ||
| filepath_config = tmp_path / 'config' | ||
| filepath_config = tmp_path_factory.mktemp('transport') / 'config' |
There was a problem hiding this comment.
Maybe add a comment for one line, why you did it this way..
probably because listdir would also list 'config' for core.auto_ssh
tests/transports/test_all_plugins.py
Outdated
| def test_put_get_abs_path_file(custom_transport): | ||
| def test_put_get_abs_path_file(custom_transport, tmp_path_factory): | ||
| """Test of exception for non existing files and abs path""" | ||
| local_dir = os.path.join('/', 'tmp') | ||
| remote_dir = local_dir | ||
| local_dir = tmp_path_factory.mktemp('local') | ||
| remote_dir = tmp_path_factory.mktemp('remote') |
There was a problem hiding this comment.
to be consistence, can you also define local_tmp_path fixture?
Because now, in some tests only use remote_tmp_path, some other directly uses tmp_path_factory ..
Either that, or I suggest just remove that remote_tmp_path fixture and always to use tmp_path
|
|
||
| # third test put. Can copy one file into a new file | ||
| transport.put(str(local_base_dir / '*.tmp'), str(remote_workdir / 'prova')) | ||
| assert transport.isfile(str(remote_workdir / 'prova')) |
There was a problem hiding this comment.
probably this is not quite the same as the former line(?)
assert set(['prova', 'local']) == set(transport.listdir('.'))There was a problem hiding this comment.
The assert I added is more strict and I think want required to be tested.
The put is to move a file to prova, the previous test only test the "moved" thing appear in the new dst folder. If a "prova" folder exist, then the previous test still passed but it is not we want.
The "local" is a side effect of using the same folder to test things. It is not expect to be exist in the remote.
tests/transports/test_all_plugins.py
Outdated
|
|
||
|
|
||
| def test_exec_pwd(custom_transport): | ||
| def test_exec_pwd(custom_transport, remote_tmp_path): |
There was a problem hiding this comment.
I would keep the previous pwd and instead make it exclusive to core.ssh & core.auto_ssh & core.local.
This test would not make sense for plugins without getcwd
There was a problem hiding this comment.
plugins without getcwd
what you mean by plugins without getcwd? I think ssh/auto_ssh/local all has this method implemented, no?
|
This PR require the change in #6639 to not fail the mypy. |
unkcpz
force-pushed
the
Remove-the-use-of-deprecated-tranport-methods-in-tests
branch
from
cc051b4
to
f4de64d
Compare
These are changes required by #6620, and some changes are overlap with changes in #6626. The changes are focus on following two purposes:
chdirandgetcwdmethods.tmp_pathandtmp_path_factoryfixtures to mock remote and local folder to test transport plugins.The change make every test has its own tmp folder therefore can run independent.