Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bytes -> str handling in linux.dig action #4993

Merged
merged 23 commits into from
Aug 11, 2020
Merged

Fix bytes -> str handling in linux.dig action #4993

merged 23 commits into from
Aug 11, 2020

Conversation

blag
Copy link
Contributor

@blag blag commented Jul 15, 2020

The linux.dig action Python script didn't work on Python 3 due to bytes/string encoding issues. This PR fixes that.

TODO:

  • Add tests?
  • Update changelog

@pull-request-size pull-request-size bot added the size/S PR that changes 10-29 lines. Very easy to review. label Jul 15, 2020
@blag blag added this to the 3.3.0 milestone Jul 16, 2020
@pull-request-size pull-request-size bot added size/M PR that changes 30-99 lines. Good size to review. and removed size/S PR that changes 10-29 lines. Very easy to review. labels Aug 4, 2020
@blag blag force-pushed the fix-dig-action branch 4 times, most recently from 5ded004 to df6ae6a Compare August 5, 2020 03:15
Copy link
Member

@nmaludy nmaludy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only thing i think you could improve upon is to add a test case where a byte string is actually returned from subprocess.

contrib/linux/tests/test_action_dig.py Outdated Show resolved Hide resolved
Copy link
Member

@nmaludy nmaludy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it! This is something we've needed for a while

@blag blag merged commit 601f272 into master Aug 11, 2020
@blag blag deleted the fix-dig-action branch August 11, 2020 22:28
@blag
Copy link
Contributor Author

blag commented Sep 15, 2020

For the netstat action there really isn’t much to do since it’s a straight remote-shell-cmd action, and it only has a single user-specified parameter, args. So this one probably doesn’t need a unit test.

Same story for the cp, mv, netstat_grep, pkill, rm, rsync, scp, and vmstat actions.

I’m a little confused why the file_touch action doesn’t use the touch command…that could probably be changed, but if you do I would rename the existing file_touch action to something like file_init.

For the service action, it’s a remote-shell-script but it’s actually a Python script, so you can probably import directly from it, mock up and monkeypatch the sys and subprocess modules in it, and then assert that the right command arguments are passed to subprocess.call.

check_loadavg needs some tests, I would mock up and monkeypatch loadAvgFile and cpuInfoFile, override sys.stdout and check its output. Once you’ve got some tests, it then needs to be updated to support both Python 2 and 3 (except Exception:).

check_processes needs tests - you don’t need to test every method, but I would test at least byState, byPid, and byName. First, choose a pid number and set the action instance's myPid property to it, then create a test fixture directory that contains directories with purely numeric names, a few with non-numeric names, and a name that matches your chosen myPid number (say: 3456. Make sure you put stat and cmdline files in each pid directory:

proc_fixture/
proc_fixture/1234/
proc_fixture/1234/stat
proc_fixture/1234/cmdline
proc_fixture/2345/
proc_fixture/2345/stat
proc_fixture/2345/cmdline
proc_fixture/3456/
proc_fixture/3456/stat
proc_fixture/3456/cmdline
proc_fixture/abcd/
proc_fixture/abcd/stat
proc_fixture/abcd/cmdline
proc_fixture/4567/
proc_fixture/4567/stat
proc_fixture/4567/cmdline

Set the procDir property to your fixture directory, override sys.stdout and check that against a known value.

For wait_for_ssh, we need to check two of the three use cases: (1) unconnectable host (SSH timeout), and (2) instantly connectable host (eg: localhost). Use test_action_dig.py as a template for this.

If you need help standing up a test environment to begin with, I would dig through the Travis config and rerun the same commands in a VM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug python3 size/M PR that changes 30-99 lines. Good size to review. status:needs tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants