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.

Sign up for GitHub

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

Jump to bottom

Skip test on all OS's but linux #36124

Merged
merged 3 commits into from
Sep 9, 2016
Merged

Skip test on all OS's but linux #36124

merged 3 commits into from
Sep 9, 2016

Conversation

twangboy
Copy link
Contributor

@twangboy twangboy commented Sep 7, 2016 •
edited by meggiebot
Loading

What does this PR do?

Skip test on anything but linux

What issues does this PR fix or reference?

https://github.com/saltstack/qa/issues/236

@cachedout
Copy link
Contributor

Why are we testing this functionality on OS X when linux_acl is not designed to work there?

@cachedout cachedout added the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label Sep 8, 2016
@twangboy
Copy link
Contributor Author

twangboy commented Sep 8, 2016

@cachedout Apparently it does work on Mac...

@cachedout
Copy link
Contributor

The Linux module in question requires getfacl which is does not ship with OSX as I understand it. Have you tested this?

@twangboy
Copy link
Contributor Author

twangboy commented Sep 8, 2016

@cachedout I'm just saying the tests are passing on OSX. I'm happy to "skipif" the whole test.

@terminalmage
Copy link
Contributor

ACLs are supported on MacOS, but they are interacted with in an entirely different way than on Linux.

For more info, see here: http://www.techrepublic.com/blog/apple-in-the-enterprise/introduction-to-os-x-access-control-lists-acls/

Whether or not one can get a unit test to pass by altering a single value is immaterial, especially when mocking is being used.

I guess I just don't understand why the proposed solution isn't simply to skip the test on non-linux platforms. It certainly seems to be the path of lesser resistance when compared to altering the test itself.

@cachedout
Copy link
Contributor

@terminalmage is correct. Attempting to alter this test so that it runs on OS X when the code under test is not designed to work there just leads to confusion and technical debt down the road.

@terminalmage
Copy link
Contributor

@twangboy it seems that your reasoning is simply "tests are passing", thus ACLs are supported. But you don't appear to have dug any further than that.

The link I posted in my previous comment is literally the first search result for mac os acls.

The __virtual__ in linux_acl, as @cachedout said, requires the getfacl and setfacl commands. A search for mac os getfacl returns enough information in the page summaries that one need not even click through to investigate to realize that there is no getfacl in Mac OS.

I could get this test to pass on Windows by changing the lines you did to have Windows paths in them. Getting the tests to pass shouldn't be more important than ensuring they're being run correctly.

@twangboy
Copy link
Contributor Author

twangboy commented Sep 8, 2016

Yep. I'll remove them.

On Sep 8, 2016 10:22 AM, "Erik Johnson" notifications@github.com wrote:

@twangboy https://github.com/twangboy it seems that your reasoning is
simply "tests are passing", thus ACLs are supported. But you don't appear
to have dug any further than that.

The link I posted in my previous comment is literally the first search
result for mac os acls https://duckduckgo.com/?q=mac+os+acls&ia=web.

The virtual in linux_acl, as @cachedout https://github.com/cachedout
said, requires the getfacl and setfacl commands. A search for mac os
getfacl https://duckduckgo.com/?q=mac+os+getfacl&ia=qa returns enough
information in the page summaries that one need not even click through to
investigate to realize that there is no getfacl in Mac OS.

I could get this test to pass on Windows by changing the lines you did
to have Windows paths in them. Getting the tests to pass shouldn't be more
important than ensuring they're being run correctly.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#36124 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AI8v_xkBRki4cXHDPLGRj3CE8XLEhbC9ks5qoDZQgaJpZM4J3Oiy
.

@twangboy twangboy changed the title Use /etc instead of /root Skip test on all OS's but linux Sep 8, 2016
@cachedout cachedout merged commit 31b2ef2 into saltstack:carbon Sep 9, 2016
@twangboy twangboy added bugfix-bckport will be be back-ported to an older release branch by creating a PR against that branch ZRELEASED - 2016.3.4 labels Sep 13, 2016
@rallytime rallytime mentioned this pull request Sep 14, 2016
Merged
@rallytime rallytime added ZZZ[Done]-back-ported-bf RETIRED The pull request has been back-ported to an older branch. and removed bugfix-bckport will be be back-ported to an older release branch by creating a PR against that branch labels Sep 14, 2016
rallytime pushed a commit that referenced this pull request Sep 14, 2016
Back-port #36124 to 2016.3 (#36296)
7296179 
* Use /etc instead of /root

* Skip test on anything but linux

* Add missing import (lint)
gitebra pushed a commit to gitebra/salt that referenced this pull request Sep 15, 2016
@jojohans
Merge remote-tracking branch 'upstream/develop' into develop
cea0776 
* upstream/develop: (92 commits)
  Switch the order of the decorator
  Back-port saltstack#36273 to 2016.3 (saltstack#36295)
  Back-port saltstack#36124 to 2016.3 (saltstack#36296)
  Fix pkg group test by passing a list instead of str
  Report the reason that a module did not load to the user via remote-ex (saltstack#36259)
  Gate the pkg.group_installed state test: not all pkg modules have group_install
  Filter out pub kwargs from cloud runner (saltstack#36178)
  Add append_newline flag for saltstack#33686 (saltstack#36273)
  Don't stop to failover if all masters not reachable.
  Schema test requires jsonschema 2.5.0 or above
  Fix pkg group test by passing a list instead of str
  Improved gitfs/git_pillar error logging
  Integration tests fixes for 2015.8 (saltstack#36262)
  Protect the dbus import for a loader stacktrace in avahi_announce beacon
  Fix for new integration test for fileserver.clear_file_list_cache
  salt.utils.gitfs: Check for existence of ssh keys
  Pylint fix
  Integration tests fixes for 2016.3 (saltstack#36263)
  Fix misspelling of "occurred" in log messages/exceptions (saltstack#36270)
  Pass the key kwarg through to the flush function
  ...
@twangboy twangboy deleted the linux_acl_test_mac branch September 16, 2016 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged ZRELEASED - 2016.3.4 ZZZ[Done]-back-ported-bf RETIRED The pull request has been back-ported to an older branch.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@twangboy @cachedout @terminalmage @rallytime