| === _yogg is now known as codyshepherd | ||
| minimal | could do with some help with a testcase, the code it calls uses util.write_file but I can't mock.patch that as the testcase also needs to use it to create a file for the test | 15:25 |
|---|---|---|
| minimal | so I'm not sure how to achieve both - to "trap" the code doing its write_file and also to use write_file in the test | 15:25 |
| blackboxsw | minimal: can you util.write_file inline in the test before calling `m_write_file = mocker.patch("cloudinit.util.write_file")` in the specific test? | 15:48 |
| blackboxsw | minimal: or you may want to just allow write_file to do what it wants and use the fake_filesystem python fixture which will retarget all util.write_file operations to a dir under /tmp | 15:53 |
| minimal | yeah I'm using helpers.FilesystemMockingTestCase to create a temp directory | 15:56 |
| minimal | let me post a gist of what I've got currently | 15:56 |
| blackboxsw | perfect, can help better then as we'd ❤️ to move away from FilesystemMockingTestCase as it's a pain, and fake_filesystem is the successor where possible | 15:57 |
| minimal | https://gist.github.com/dermotbradley/731f0d3bbfabeaa3aa824acb179d30f0 | 16:00 |
| minimal | so function "test_busybox_add_user" is the testcase in question | 16:01 |
| minimal | on line 36 it creates a shadow file using util.write_file | 16:01 |
| minimal | on line 44 the create_user code will also call util.write_file (to modify shadow file) | 16:02 |
| minimal | so I just replace the testcase's util.write_file call with defining "content" to specify both the filename and the content and then call populate_dir accordingly? | 16:06 |
| blackboxsw | looking | 16:06 |
| blackboxsw | minimal: I'd look at something like the following if possible https://dpaste.com/7LVC45BKR. Note that this test is still going to hit a runtime error as it mocks out all of subp with a return of False | 16:29 |
| blackboxsw | but it gets you far enough into the distro.lock_passwd method | 16:29 |
| blackboxsw | but it looks like that unit test is trying to validate the impact of actually calling passwd or usermod --lock maybe? | 16:31 |
| minimal | the testcase is for Busybox's adduser (rather than "useradd") but the code also "manually" modifies /etc/shadow as well as adduser does not provide the ability to set some /etc/shadow fields | 16:32 |
| blackboxsw | minimal: since this test case is mocking out all subp calls. nothing is actually invoking busybox right? So, we'll never get down to your actual validation of the /tmp/etc/shadow being changed will we? | 16:37 |
| blackboxsw | This test intent is just to validate that the calls that would be going to busybox have the correct params on the CLI. | 16:37 |
| blackboxsw | Is that true? | 16:38 |
| minimal | hmm, true....as adduser isn't actually run then nothing is written to /etc/shadow, but the code will still then try and update the (non-existent) entry in /etc/shadow....I guess I can just add that to the testcase's generated /etc/shadow contents | 16:39 |
| minimal | the actual code uses util.load_text_file and util.write_file to manipulate /etc/shadow | 16:40 |
| blackboxsw | for my context, this is relate to https://github.com/canonical/cloud-init/pull/5176 I presume? | 16:40 |
| -ubottu:#cloud-init- Pull 5176 in canonical/cloud-init "feat(alpine): add support for Busybox adduser/addgroup" [Open] | 16:40 | |
| minimal | yupe | 16:40 |
| minimal | I'm just finishing up with some additional changes and this is the last testcase to sort out | 16:41 |
| blackboxsw | ok roger. switching to your PR so I can get bearings on the test need | 16:42 |
| blackboxsw | minimal: https://dpaste.com/AY6H5XMYR maybe? validate the commands you think should be called | 16:51 |
| blackboxsw | the above passes on your branch | 16:51 |
| blackboxsw | and additionally you'd want to load that shadow_file afterward to ensure the write_files worked... I think you your PR you'll maybe want to use util.load_text_file instead of a direct call to open(), so we can log what cloud-init reads from the filesystem | 16:52 |
| blackboxsw | I'll go through that PR now and add a couple of review comments | 16:53 |
| minimal | yeah I've already changed the code to use util.load_text_file | 16:53 |
| minimal | there's no point reviewing it yet until I push my additional code changes | 16:54 |
| blackboxsw | sounds good, note that the restructure of the unittest in question doesn't setup expirydate or inactive as not-None, so /etc/shadow isn't altered in the tmpdir | 17:02 |
| blackboxsw | will let you work out additional code changes before peeking again | 17:02 |
| minimal | blackboxsw: /etc/shadow is altered because of some additional changes I've made locally ;-) | 17:02 |
| blackboxsw | I've misplaced my ssh keys to your home system and bank accounts etc. So, I'll just await a ping or notification on the PR : | 17:03 |
| blackboxsw | :) | 17:03 |
| minimal | blackboxsw: thanks for your help, making progress, now having testcase fail for another reason (going to have to handle mocking of subp calls for "addpasswd" and "passwd" separately) | 17:17 |
| minimal | as code's subp call of "passwd --help" expects stdout/stderr output | 17:18 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!