Mocked SSH server and enable ssh test on windows #1911

ag613915cao · 2019-04-21T09:48:09Z

Keep current SSH remote connection test on real system
Faked and Mocked SSH server which running on both Linux and Windows
Add corresponding tests

tests/func/test_data_cloud.py

ag613915cao · 2019-04-22T11:55:59Z

@efiop I have just supported Mocked SSH server which is not rely on OS ssh binary. And the test case on Linux is passed.
Will to check with windows OS case soon.
Meanwhile, could you please check if it is the right way? Thanks

ag613915cao · 2019-04-23T12:54:57Z

@efiop Do you know why it is timeout for walk_files while running the test on Windows [1]? I have check the NOTE but not quite understand [2]

[1] https://ci.appveyor.com/project/iterative/dvc/builds/24039752#L2917
[2] https://github.com/iterative/dvc/blob/master/dvc/remote/base.py#L295-L299

efiop · 2019-04-23T13:38:53Z

@ag613915cao I believe that timeout is not related to that note in our code. You should probably simply try increasing the timeout in tests/__main__.py. At least that would be the first thing that I would try 🙂

ag613915cao · 2019-04-23T13:49:50Z

@ag613915cao I believe that timeout is not related to that note in our code. You should probably simply try increasing the timeout in tests/__main__.py. At least that would be the first thing that I would try 🙂

Thanks @efiop . I tried to increased timeout from 600 to 1800 but it is still failed. Let's me try to increase it more to see.
For the source, I tried to debug and it hang at paramiko.buffered_pipe in read method. Maybe the file is too big with the timeout.

efiop · 2019-04-23T14:36:03Z

@ag613915cao got it. It was simply my first instinct 🙂 Can't say what happened here right away, need to take a closer look 🙁

efiop · 2019-04-24T03:19:32Z

@ag613915cao Timeout of 9000 is way too much :) I was wondering if maybe increasing it a bit would help, but waiting for several hours to run this test is not going to tell us anything. Btw, please rebase on top of master, there were a few changes related to walk method, maybe they will help.

ag613915cao · 2019-04-24T03:40:11Z

@efiop Indeed! The reason I set it to 9000 to see if it is move forward. Because I couldn't debug it in local without any hints or source of reason.
I have just rebased on top of latest master branch (I just rebased it couple hours ago and didn't aware of new landing patch related to walk method) to see if it help (and timeout is set to 3600 for reasonable).

ag613915cao · 2019-04-24T09:04:19Z

@efiop After prepared local environment I found the root cause. It is not related to our code base as you mentioned. It is just because prefix in Windows contain too much dirs and files for processing (especially C:\Windows directory).
I have updated the code (no need to extend timeout) and it is ready to go.
So, kindly please take a look. Thank you

dvc/remote/ssh/__init__.py

dvc/remote/ssh/connection.py

tests/func/test_data_cloud.py

tests/func/user.key

tests/func/user.key.pub

ag613915cao · 2019-04-25T06:26:47Z

@efiop I have updated the patch based on your comments. Kindly please take a look and let me know if it need to update something. Thanks a lot

ag613915cao · 2019-04-25T14:46:58Z

Just resolved merge conflict (no logic change)

dvc/remote/ssh/__init__.py

ag613915cao · 2019-04-25T15:43:26Z

@efiop I updated the patch with your suggestion :) Kindly please help to take a look and land it if possible.

tests/func/test_data_cloud.py

tests/unit/remote/ssh/test_ssh.py

tests/func/test_data_cloud.py

ag613915cao · 2019-04-26T03:49:51Z

@efiop I updated the patch as you requested. Kindly please take a look and let me know if it needs to update something. Thanks a lot :)

tests/conftest.py

tests/func/test_data_cloud.py

ag613915cao · 2019-04-26T10:21:47Z

@efiop I think all the points has been addressed :)

tests/func/test_data_cloud.py

tests/unit/remote/ssh/test_ssh.py

- Keep current SSH remote connection test on real system - Faked and Mocked SSH server which running on both Linux and Windows - Add corresponding tests Fixed #1704

efiop

Thanks! 🙂

ag613915cao · 2019-04-26T11:26:15Z

@efiop I'm happy to work with you. :)

Suor · 2019-04-26T14:17:35Z

@ag613915cao @pytest.yield_fixture is deprecated now, you use normal @pytest.fixture, it sees yield and does the right job.

ag613915cao · 2019-04-26T15:28:49Z

@Suor I don't know who you are and don't like this working style. If you feel it needs to improve, please open new issue. Or at least, please provide feedback before the patch is merged.
You just gave your comment without any evidence/reference. That is not good in my opinion. Thanks!

shcheklein · 2019-04-26T17:31:30Z

tests/conftest.py

+key_path = os.path.join(here, "{0}.key".format(user))
+
+
+@pytest.yield_fixture()


Hi @ag613915cao , I think @Suor was referring to using this decorator instead of just @pytest.fixture. See the example in this file, a few lines above.

@shcheklein Yes. I know about that. What I mean here is the way reviewer provide feedback to contributor(s). I have at least 5 years contributing to open source projects. I can realize that.

For this point: I will update this within today. Thank you for your consideration.

Suor · 2019-04-27T06:26:02Z

@ag613915cao I didn't mean to offend you. I saw a deprecated decorator, rechecked it and posted the comment here for this to not get lost.

I understand that it would be easier if I'd posted this before the PR got merged, but I only saw it after.

ag613915cao · 2019-04-29T01:07:09Z

@Suor Sure. I will update the point within today to make you feel satisfaction. Thank you.

Marking functions as yield_fixture is still supported, but deprecated and should not be used in new code.

[1] https://docs.pytest.org/en/latest/yieldfixture.html

ag613915cao mentioned this pull request Apr 21, 2019

Add test for ssh connection #1908

Merged

efiop reviewed Apr 21, 2019

View reviewed changes

tests/func/test_data_cloud.py Outdated Show resolved Hide resolved

ag613915cao changed the title ~~This commit intends to enable ssh test on windows~~ [WIP] This commit intends to enable ssh test on windows Apr 22, 2019

ag613915cao changed the title ~~[WIP] This commit intends to enable ssh test on windows~~ Mocked SSH server and enable ssh test on windows Apr 24, 2019

efiop reviewed Apr 24, 2019

View reviewed changes

dvc/remote/ssh/__init__.py Outdated Show resolved Hide resolved

efiop suggested changes Apr 24, 2019

View reviewed changes

efiop reviewed Apr 25, 2019

View reviewed changes

dvc/remote/ssh/__init__.py Outdated Show resolved Hide resolved

efiop reviewed Apr 25, 2019

View reviewed changes

tests/func/test_data_cloud.py Outdated Show resolved Hide resolved

efiop reviewed Apr 25, 2019

View reviewed changes

tests/unit/remote/ssh/test_ssh.py Outdated Show resolved Hide resolved

efiop reviewed Apr 25, 2019

View reviewed changes

tests/func/test_data_cloud.py Outdated Show resolved Hide resolved

efiop reviewed Apr 26, 2019

View reviewed changes

tests/conftest.py Outdated Show resolved Hide resolved

efiop reviewed Apr 26, 2019

View reviewed changes

tests/func/test_data_cloud.py Outdated Show resolved Hide resolved

efiop suggested changes Apr 26, 2019

View reviewed changes

tests/func/test_data_cloud.py Outdated Show resolved Hide resolved

efiop reviewed Apr 26, 2019

View reviewed changes

tests/func/test_data_cloud.py Outdated Show resolved Hide resolved

efiop reviewed Apr 26, 2019

View reviewed changes

tests/unit/remote/ssh/test_ssh.py Outdated Show resolved Hide resolved

efiop reviewed Apr 26, 2019

View reviewed changes

tests/unit/remote/ssh/test_ssh.py Outdated Show resolved Hide resolved

This commit intends to:

fd385c9

- Keep current SSH remote connection test on real system - Faked and Mocked SSH server which running on both Linux and Windows - Add corresponding tests Fixed #1704

efiop approved these changes Apr 26, 2019

View reviewed changes

efiop merged commit 3a183f5 into treeverse:master Apr 26, 2019

shcheklein reviewed Apr 26, 2019

View reviewed changes

		key_path = os.path.join(here, "{0}.key".format(user))


		@pytest.yield_fixture()

Mocked SSH server and enable ssh test on windows #1911

Mocked SSH server and enable ssh test on windows #1911

Uh oh!

Conversation

ag613915cao commented Apr 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ag613915cao commented Apr 22, 2019

Uh oh!

ag613915cao commented Apr 23, 2019

Uh oh!

efiop commented Apr 23, 2019

Uh oh!

ag613915cao commented Apr 23, 2019

Uh oh!

efiop commented Apr 23, 2019

Uh oh!

efiop commented Apr 24, 2019

Uh oh!

ag613915cao commented Apr 24, 2019

Uh oh!

ag613915cao commented Apr 24, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ag613915cao commented Apr 25, 2019

Uh oh!

ag613915cao commented Apr 25, 2019

Uh oh!

Uh oh!

ag613915cao commented Apr 25, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ag613915cao commented Apr 26, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ag613915cao commented Apr 26, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

efiop left a comment

Choose a reason for hiding this comment

Uh oh!

ag613915cao commented Apr 26, 2019

Uh oh!

Suor commented Apr 26, 2019

Uh oh!

ag613915cao commented Apr 26, 2019

Uh oh!

shcheklein Apr 26, 2019

Choose a reason for hiding this comment

Uh oh!

ag613915cao Apr 29, 2019

Choose a reason for hiding this comment

Uh oh!

Suor commented Apr 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ag613915cao commented Apr 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

ag613915cao commented Apr 21, 2019 •

edited

Loading

Suor commented Apr 27, 2019 •

edited

Loading