16:59:58 #startmeeting Testing Working Group 16:59:58 Meeting started Thu Nov 16 16:59:58 2017 UTC. The chair is mattclay. Information about MeetBot at http://wiki.debian.org/MeetBot. 16:59:58 Useful Commands: #action #agreed #halp #info #idea #link #topic. 16:59:58 The meeting name has been set to 'testing_working_group' 17:00:10 #info Agenda: https://github.com/ansible/community/issues/248 17:00:36 Hello 17:00:40 #chair sivel 17:00:40 Current chairs: mattclay sivel 17:01:03 Hi 17:01:21 #chair aalexmonteiro 17:01:22 Current chairs: aalexmonteiro mattclay sivel 17:02:13 * mattclay waits a few minutes for others to show up 17:02:37 Hi 17:02:47 Builder is here, so only half in IRC at the moment 17:02:51 Pilou: You around? 17:03:08 #chair gundalow 17:03:08 Current chairs: aalexmonteiro gundalow mattclay sivel 17:05:12 There is only one item on agenda, but we'll wait for Pilou to discuss. 17:05:37 In the meantime, is there anything else to discuss? 17:06:28 I think there are 2 things in regards to the PR that Pilou wants to discuss 17:06:31 one we can discuss now 17:06:50 that is around no longer accepting new tests using unittest, and requiring pytest style tests 17:07:00 that is not yet documented, but we have been enforcing it in reviews and such 17:07:22 My initial assumption was that we were going to have Pilou document that, but I believe his understnading and desire was otherwise 17:07:33 But that does impact the scope of his PR 17:07:34 * Pilou waves 17:07:39 #chair Pilou 17:07:39 Current chairs: Pilou aalexmonteiro gundalow mattclay sivel 17:08:15 sivel: using pytest hasn't been enforced for network unit tests 17:08:33 it should be though, so if we aren't we are failing at enforcing that 17:08:42 all new tests should be pytest per our last decision 17:08:50 but yes, it isn't documented 17:09:10 so I think we need to re-solidify that, and get it out there 17:09:35 by the way I didn't add any new test, i updated existing tests 17:10:30 yes, it was more around the documentation, which I undertand you removed. Although we are putting a lot into code that we plan on having to rewrite anyway later to use pytest style 17:11:04 then should I consider this PR rejected ? 17:11:52 I'd like mattclay and gundalow to weigh in here 17:12:56 Pilou: I think your PR does add value as it's reducing redundant code within the tests. 17:14:54 I don't expect we're going to have anyone volunteer to go through and re-write all of our existing unit tests to switch everything over to pytest. So it seems reasonable to clean up code duplication within the existing unit tests, even if we'd like to see them converted to pytest style eventually. 17:15:24 Even if someone does convert them, I think the cleanup work that Pilou has done will still be useful. 17:15:34 I am +1 to that 17:15:52 I don't know where my priorities will get shifted, but I do plan on spending some time in tests 17:16:02 though potentially increasing coverage at first 17:18:09 I haven;'t specifically reviewed the PR, so I can't speak on anything past what mattclay has stated 17:18:56 When creating new unit test files it makes sense to use pytest style. When adding tests to an existing file (or just fixing those tests) I can understand wanting to stick to the style that is already present. I don't think we should insist on converting to pytest style whenever an existing test file is touched. 17:19:32 Yes, I may not have been clear about "new tests", but I was indicating effectively new files or large portions of tests 17:20:57 Hi 17:21:21 I think of the conversion to pytest as similar to PEP 8 changes. Don't make changes to the style of existing tests in the same PR as non-style changes are being made. The two should be done separately. 17:22:00 Regarding the network unit tests (which will be growing) I'm happy to 17:22:12 1) Tell people, including partners to do it a different way 17:22:30 2) work through the backlog and update the existing tests - IF there is a benefit 17:23:21 Pilou: At this point, what questions do you have on where things should be going with your PR? 17:23:35 For brand new tests MUST they be done as pytest (ie hard requirement?) 17:24:14 Although in many cases switching to pytest would allow simplifying the code so I wouldn't be against people deciding they want to port just beause. 17:24:24 #chair abadger1999 17:24:24 Current chairs: Pilou aalexmonteiro abadger1999 gundalow mattclay sivel 17:24:25 mattclay: the "policy" is clear to me now :) 17:24:30 abadger1999: Good info, thanks 17:24:48 So I would allow someone to change the test and convert at the same time. 17:24:50 gundalow: I believe yes, "new tests" must be pytest 17:26:22 abadger1999: In cases like that it would probably be easier to review if the conversion of the existing tests was at least done in a separate commit in the same PR. 17:27:19 Pilou: Did you see the comment on your PR from mikedlr? https://github.com/ansible/ansible/pull/31456/files#r143680842 17:27:27 OK, so 17:27:34 mattclay: yeah... probably. But the only reason is that the existing test is probably harder to read than the pytest version. 17:27:55 1) Docs need updating to make it clear that all *new* unit tests should be pytest 17:28:22 mattclay: so i'm not sure i'd make a hard and fast rule around that. 17:28:22 mattclay: i can make this change 17:28:23 2) Docs: PRs welcome to update existing tests to use pytest, though that should be the only change in the PR otherwise it's too difficult to read 17:28:44 better rule is maybe "Change less functions/tsts in each commit" 17:28:58 Do we have an example PR of a port to pytest, how much of a change is it? 17:29:08 * gundalow has to step away again 17:29:46 gundalow: some things are very simple other things are more in-depth. the more in-depth ones are where pytest proves its worth, though. 17:30:04 Yeah. The idea of splitting conversion from actual test changes is all about making it easier to review. I consider it more of a recommendation than a hard rule. Of course if the PR is more difficult to review it's likely going to take longer to get reviewed. 17:30:22 I might be able to dig up some conversion of module_utils tests that shows the complex-but-more-readable-after things. 17:31:08 abadger1999: I think that would be helpful for a number of people. I personally don't have a huge amount of experience with pytest 17:31:09 gundalow: For #1 we need to be clear on what is considered "new tests". 17:31:55 sivel: the two things that pytest does much better than unittest are fixtures and parameterization. 17:31:57 gundalow: AFAIK there isn't an example of a port to pytest. There are some cloud/amazon tests using pytest. 17:31:59 I would consider new tests, new files containing tests, or a large number of tests contributed to an existing file. Although defining "large number" is semi subjective 17:32:12 like 12 :) 17:32:39 unittests fixtures are very naive compared to pytest and unitest parametrerization isn't builtin. 17:34:03 sivel, gundalow: https://github.com/ansible/ansible/commit/47f26ee11c84b28e3eac0379c9c3e3112de1b32d 17:34:23 abadger1999: I tried to switch my current $work tests to pytest once, and we have a guy on the team who hates "frameworks" and other simialr things, and wanted something that didn't force him into using a test running outside of what was available in the stdlib ¯\_(ツ)_/¯ 17:34:23 sivel: I tend to think of "significant percentage" instead of "large number". So if new tests in an existing file end up being something like 80% percent of the lines in the file, it's probably time to convert the rest. 17:35:03 sivel: unittest is more frameworky than pytest but it does have the advantage of being shipped in the stdlib. 17:35:24 I was more saying, if you are adding a bunch, don't make them part of the existing, write them as pytest, leaving the previous as is unless you care to rewrite them 17:35:42 we just need to clarify which direction to go 17:35:49 mattclay: note that a lot of times with the stuff I'm working on I also end up moving the pytest code to a new file. 17:36:23 abadger1999: that is preobably a good idea, although what are you doing as far as naming is concerned (of the new file)? 17:36:32 abadger1999: Is that because the existing test file is incorrectly named for what it is testing? 17:36:45 pytest lets you easily test a lot more cases so my tests go from a test file per lib/ansible file to a test file per lib/ansible "function" (really functionality) 17:37:26 sivel: for module_utils/basic.py I went from units/module_utils/test_basic.py to units/module_utils/basic/test_$FUNCTION.py 17:37:45 ok, that make sense I suppose, I've never broken tests out that far before 17:37:57 abadger1999: Yeah, that makes a lot of sense. 17:37:57 You can look at the commit I linked to to see what I'm doing. 17:37:59 but I suppose ti also keeps your files from being thousands of lines long 17:38:27 Just as long as we're following a consistent pattern so it's easy to match up unit tests to what they're testing. 17:38:34 sivel: yep. Easier to see. Also, because you can get very declarative via parametrization, it keeps the data and the test code close together. 17:40:01 I have used classes in pytest just to organize as well. That works okay for seveal small functions which are related but I wouldn't recommned it for stuff that's more unrelated. 17:40:37 (AnsibleModule, for instance would be monstrous to test that way. But module_utils/_text might be a good candidate) 17:40:56 So back to Pilou's PR. Is there anything that needs to be changed or added there? 17:42:24 i noted s/TestAnsibleModule/ModuleTestCase/ 17:42:27 From a high level I don't think so, but we just need some other docs around pytest, but that isn't something Pilou needs to do 17:45:02 Does anyone want to volunteer to work on those docs updates? 17:45:10 gundalow: ./test/units/modules/cloud/openstack/test_os_server.py seems a good sample of pytest based 17:45:41 I don't feel comfortable taking that doc work on, with my limited exposure to pytest 17:47:11 mattclay: I propose that i update some unittest based tests I wrote using pytest. Once reviewved and merged then i would be able to update documentation 17:47:51 +1 17:48:04 but a minium, at least what gundalow mentioned, should be added sooner rather than later 17:48:20 just my opinion 17:48:34 effectively that new tests should use pytest 17:48:52 #info all *new* unit tests should be pytest 17:48:53 sivel: Yeah, we can do something quick now and then come back and update docs with actual examples later. 17:49:01 I don't know if gundalow was going to add that or just communicate it to partners 17:49:27 #info PRs welcome to update existing tests to use pytest 17:51:45 #action Pilou update an existing test, replacing unittest with pytest. Once merged, update the documentation. 17:52:11 Pilou: Once you update your PR with the `ModuleTestCase` name change I'll re-review and merge. 17:53:11 Pilou: We'll want to get sign-off on the docs changes though before merging. 17:53:33 The docs changes are minor so they should be reviewed quickly. 17:54:44 i removed the doc changes 17:54:46 back 17:55:55 Pilou: I still see minor docs changes in the PR: docs/docsite/rst/dev_guide/testing_units_modules.rst 17:56:16 oh you're right 17:56:59 For policy stuff, I've found it works best if we document in dev_guide first. I'm going to be doing an email to Network Partners next week, so I can include a link in that 17:57:00 they are unrelated to pytest/unittest, I will move them in another PR 17:59:17 OK, we're almost to the end of our scheduled meeting. Is there anything else we need to discuss? 17:59:47 Do we know how we are progressing this? 18:01:15 ah, missed te #action above 18:01:19 Thanks mattclay :) 18:01:35 #info NO MEETING NEXT WEEK 18:04:28 #endmeeting