| === tds1 is now known as tds | ||
| blackboxsw | minimal: looking through this now. Also going to write some test cases to exercise the test_distro_empty_password which will actually exercise the various subp grep logic to ensure we we see success/failure based on what distro defines as shadow match substrings. | 15:41 |
|---|---|---|
| blackboxsw | that kind of behavior where pre-existing tests start failing when new tests are added typically means a either mock or fixture that is too broad or not being torn down properly in the pre-existing test is being affected by additional mock values added in new unittests. I try making certain that mocks are generally scoped at as close to the module as possible to avoid potential interaction with other module tests. | 15:44 |
| blackboxsw | For example mocking the module-local import path 'cloudinit.config.cc_<somemodule>.util.load_binary_file' instead of 'cloudinit.util.load_binary_path' | 15:45 |
| blackboxsw | minimal: If there is something in-flight you'd like to push to your PR I can look that while I write up a supplementary unittest that validates greps of shadow file content. | 15:47 |
| blackboxsw | typo: I can look *at* that | 15:48 |
| minimal | blackboxsw: the grep logic tests sound good, I've been struggling as to how to handle the _check_if_existing_password boolean return code | 15:49 |
| blackboxsw | I'm also thinking of flipping the logic and return codes of _check_if_existing_password method to be named _check_user_has_empty_password because that method isn't really asserting that a user has a password. It's asserting that they don't have an empty locked/empty password | 15:51 |
| blackboxsw | for example, users not present at all in /etc/shadow would still get a True from _check_if_existing_password | 15:51 |
| blackboxsw | so the name is a bit misleading | 15:52 |
| minimal | true | 15:52 |
| minimal | ok, so rather than bashing my head more with failing tests I think I should do the rebase (plus your suggesting change from yesterday) and then take things from there | 15:54 |
| blackboxsw | please do. Or wait an hour and I can push a changeset directly to your PR that you can review/adapt if you want. | 15:55 |
| minimal | whichever suits you | 15:55 |
| blackboxsw | ok it may make things easier if I can dump the suggestions I have directly into your PR in the next hour. Then we can address any test coverage gaps or different suggestions necessary. | 15:57 |
| minimal | ok then | 15:57 |
| blackboxsw | I'll ping when I think I'm done. thank you for your patience | 15:57 |
| blackboxsw | minimal: I also am thinking about avoiding grep in this case because we also want to assert the username is in the shadow file in the first place, and shelling out with subprocess to grep for simple pattern matching as well as grepping for the presence of a username is maybe a bit costly. I'll have an example in 30 mins | 16:21 |
| minimal | ok | 16:22 |
| blackboxsw | minimal: taking a break for lunch, and pushed a work in progress commit for your review/comments and an approach I'm working towards. I can clean up the comments in an hour | 18:44 |
| minimal | blackboxsw: looks good, only a couple of minor points, in test_create_users.py the _existing_shadow_grep function can now be removed, also perhaps rename the test id "no_unlock_in_snappy_on_empty_locked_user_passwd_in_extrausers" to "no_unlock_in_snappy_on_locked_empty_user_passwd_in_extrausers" to fit in with the id name above it (swapping the order of "empty" and "locked" in the name) | 20:48 |
| blackboxsw | thanks minimal just force pushed that commit again right now with a fix or two and more unittests | 20:49 |
| minimal | I'll have to rework some of the BSD (test_*bsd.py) testcases I have here (or just get rid of them) as they're expecting a "grep" | 20:49 |
| blackboxsw | +1, I have a school function to step away to but will be back checking in later. would like to wrap up what we can on this tomorrow if possible. | 20:49 |
| blackboxsw | ideally this is one of the last things to get into 24.3. | 20:50 |
| === blackboxsw is now known as blackboxsw_away | ||
| minimal | blackboxsw: I assume the patterns in openbsd.py would also need to be escaped | 22:02 |
| minimal | blackboxsw: re the freebsd tests, you've got 2 with same id "no_unlock_on_empty_locked_user_passwd_freebsd", I think also the 1st of those should have password field of "::" rather than ":*:" (3rd freebsd also has ":*:"), and also missing "_format3_" id. | 23:44 |
| minimal | also both netbsd tests have same id (no need for 3rd test with "::"?) | 23:46 |
| minimal | also shouldn't the BSD tests be referring to /etc/master.passwd rather than /etc/shadow? | 23:54 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!