On 29.02.2016 08:07, Ancor Gonzalez Sosa wrote: [...] Oh, my, where to begin, where to end... Let me start by saying that there are different styles of working, different approaches to do things, even considering that it all revolves around an object oriented approach. We don't need to discuss that we all want code that not only works (this is the most basic requirement, of course), but that is also easy to use, easy to maintain, adequately documented and testable. I think we all agree on those points. There seem to be different schools of thought, though, how to achieve that.
1) RSpec as a design validator
I like the RSpec approach to unit testing and I consider the RSpec tests of a class like the detailed documentation about its behavior.
RSpec is one approach to testing, and it makes a claim - a very bold claim - that it also serves as a substitute for a formal specification. While I agree that it is a test tool that does its job reasonably well, I dispute the claim that it can serve as documentation. It just doesn't. Try reading somebody else's RSpec code without having read documentation of the class that it tests, without having read an example. You will not figure out what the hell that class does. You may find out what some fringe cases are all about, but there is just no way to get a reasonable overview what the class is all about. You get swamped by a flood of details before you get to see the overall picture. So, IMHO RSpec fails miserably on its second claim - acting as specification. The trouble is that it forces the developers to be a lot more verbose during writing the tests than should be necessary; the rationale for that is the "it's also a spec" part which IMHO just does not materialize. But maybe that's just me; I'd be interested in other opinions about that. Please notice that I am all in favour in writing and having tests. This is a must-have to ensure covering all major use cases and a lot of fringe cases, and it's a prerequisite for avoiding regressions in the future. It's costly, but it will (hopefully) be a worthwhile investment. But there is a caveat: However hard we try, there will never ever be 100% test coverage. There will always be untested scenarios. This is not to argue against unit testing - to the contrary; it's just to avoid believing that this will solve or avoid all possible problems. For example, take that DiskSize class from libstorage-ng. It's a pretty small class that can handle disk sizes in MiB, GiB, TiB etc., and I added an RSpec test suite prette early while writing it. I thought it was pretty complete and had pretty good coverage. "Pretty" is the commanding word here. "Pretty" means "not 100%", "not perfect". And not 100% perfect it was: Only when using that class extensively I found that there were still some aspects missing, despite all that testing. For example, I got output formatted as "1024.0 GiB" when I expected "1.0 TiB" - the classic off-by one error when going up to the next-higher unit - comparing for < 1024 when it should have been <= 1024. Now, that was easy to fix, but it demonstrates what I wrote above: However complete you think your test suite is, it will always miss some important test cases. You learn that over time while using a class. A test suite will always be work in progress; it will never be complete as long as the code it tests is in use.
2) Another note about OOP design
In our new code there are some things that I consider kind of "design smells". I can be, as always, wrong but I feel like some of our code is misusing OOP as a mere way of encapsulate functionality. Going back to concrete real examples, we had this:
fake_probing = Yast::Storage::FakeProbing.new devicegraph = fake_probing.devicegraph factory = Yast::Storage::FakeDeviceFactory.new(devicegraph) factory.load_yaml_file(input_file) yaml_writer = Yast::Storage::YamlWriter.new yaml_writer.write(devicegraph, output_file)
The fact that we had a "new" every couple of lines is a design smell to me. The adopted solution was creating class methods, so we can do things like this.
fake_probing = Yast::Storage::FakeProbing.new devicegraph = fake_probing.devicegraph Yast::Storage::FakeDeviceFactory.load_yaml_file(devicegraph, input_file) Yast::Storage::YamlWriter.write(devicegraph, output_file)
Just a cosmetic fix for an issue that I consider deeper: we are writing good old procedural code dressed up as OO code. Every class has the only responsibility of implementing one step in a process and only one public method to execute that step (manipulating a set of I/O arguments).
I agree (and as a matter of fact, I even wrote that in that new functions' description) that it's just cosmetic to use a class method (or, in Ruby lingo, "singleton" method) to avoid the one-trick-pony use. I do not agree that this in any way violates the object-oriented approach - to the contrary. (more about this below)
Is just one example of solutions that are perfectly correct but look quite unfamiliar to a Ruby developer's eyes and are quite hard to express as a set of RSpec behavior-oriented specs. Not saying they are wrong, just that they don't look like "the ruby way". Like this alternative which (although not perfect) is a much more "rubyist" way of writing the same:
factory = Yast::Storage::DevicegraphFactory.new devicegraph = factory.create_from_file(input_file) File.open(output_file, 'w') {|f| f.write devicegraph.to_yaml }
Again, this is an approach I don't like at all because it focuses on the wrong thing: All that stuff is not about creating device graphs. We are not in the business of making or selling device graphs. Device graphs are just a tool for what we really intend to do. Concentrating on an irrelevant (or, to put it less harshly, behind-the-scenes) tool is most misleading for anybody looking at that code. Worse, putting the burden of file handling to the caller IMHO is not desirable; why would I as a caller want to bother with that file? I want a function that does that for me, including error handling and whatnot. Worse still, you need to obey a call protocol when you handle it like that; you introduce boilerplate code that everybody will have to copy verbatim from some other place where it works. Back in the early 1990s when I started programming X11 and OSF/Motif, we had to write tons of boilerplate code. Change any little thing, and you are doomed; it fails in some crazy way, and you'll never know why. And you couldn't even comment out stuff to find out when it starts to fail; every little bit of that boilerplate was needed to make it work. Been there. Done that. Got the f*ing T-shirt. As a matter of fact, got a whole dressing cabinet full of those T-shirts. Want any? ;-) ("I was sent to software never-never land, and all I got is this lousy T-shirt"). Back to the point I promised earlyer: Object oriented programming. What constitutes an object, a class? - Some data (however minor) it keeps around to do its thing - Some service it provides based on those data - Encapsulating internal stuff to ensure outside independency of implementation details - Optionally, being able to inherit from that class and to reimplement (overwrite) certain aspects of how it works. And now we have those "one-trick-ponies", those classes that perform only one job, the classes that can in the most common use case be hidden in a single function. Is that a contradiction to the above? Well, IMHO no. For one thing, it's not really only that one thing it does; that one thing is just the most common use case. But if such a class is written well, it also gives you access to other methods that perform just one aspect of its main task, or that lets you perform that main task in different ways, or that lets you specify additional parameters to that main task or to any of the sub-tasks. Encapsulation is the main thing. IMHO it's the fine art of OOP to do most things with useful parameters without having to write tons of that boilerplate code mentioned above. Only when you want to do the nonstandard use case you should have to specify parameters, or at least most of them (there is not always a good default; filenames are a prime example). Take the YaST UI, for example. By default, you don't have to specify anything; just get the widget factory from the UI, and then you can merrily begin creating widgets. No boilerplate code needed at all. But if you want a special setup, use different init calls and then get the widget factory and create widgets. To the outside, this looks like procedural code, but it's really not. It carries its state inside, and it uses reasonable defaults for the most common use cases. And this is similar to the classes in yast-storage-ng. It might not always show, but this is the philosophy behind it. The defaults should always be reasonable, but still it leaves you room to tweak a lot of things. And, more importantly, it hides the gory details from the outside. In many cases, however, those gory details are accessible enough for the caller or for derived classes. For example, we had this discussion on IRC last week that Josef wanted minimalistic YAML files for those device trees and auto-generate partitions and file systems because his use case just doesn't care about most of that. While this would not be desirable for the normal case IMHO, I pointed out to him that it's trivial to add that functionality by inheriting from the standard FakeDeviceFactory (which can read YAML files) and reimplement one or two methods to add that functionality. This is a lot harder with purely procedural code. A lot of things are happening behind the scenes in those classes, and you shouldn't need to know about that when you just want to use them. But you can if you want, and there is support for nontrivial use cases. But the main use case is and should be simple. This is an important design goal IMHO.
3) Write every piece of code as something to maintain
This is more related to Scrum than to Ruby or OOP. Since we are using Scrum to create YaST, I expect most PBIs to result in a YaST client that can be invoked using /sbin/yast2 or in a set of RSpecs that will be executed in every "rake test:unit" and used as a reference documentation in the future.
By all means, yes.
I consider tests in the "examples" directory to be dead code.
I pointed that out to you before: Those examples are not tests. They are examples. They serve as documentation to show how a class is used, and they are an important development tool to work on that stuff, to have easy access to those classes without going through the hassle of starting an installation (possibly with creating a package and a driver update disk with that package and integrating that DUD into an installation ISO and starting an installation in a virtual machine). I wrote those examples mostly for myself as development tools. If you take them away, all you will achieve is that I will simply no longer share them and keep them to myself. But since we are a team, I figured that I'd like to share all development tools to make life easier. Life as a YaST developer is hard enough with all those fragile and over-complex tools we have to use on an everyday basis. We can very well use some simple tools to make life easier. I know I repeat myself, but those examples are not intended as a substitute for unit tests. We need unit tests on all levels we can get. But we also need simple examples to have a realistic chance to work with our own code.
I consider that moving code from /test or /src/clients into /src/examples is the first step to have it broken with nobody ever noticing.
Anything in src/clients that is not simply invokable by itself is dead code indeed. If I need yet another external binary to invoke something written in a scripting language, something is very wrong. Maybe we should reconsider that approach and try to get rid of that additional binary.
A have a similar feeling about code used for debugging.
As long as a piece of code is not 100% perfect (which will never happen), there will be the need for debugging code. And since debugging code tends to solve a temporary problem that is only valid for a very limited time, this is very much a moving target. If...
It should be "in the right place" design-wise. Don't think in the debugging code as something temporary. If you are needing it today, you will probably need it (or something very similar) also in the future. Make it part of your design...
...this is the claim, I'd no longer call it debugging code. Then it becomes part of the interface, and it needs the same amount of testing amd maintenance, too. But look at it from another side: If we take the "agile" approach seriously, things like debugging code are not graven in stone. It will change as development progresses. What is useful debugging code today, it might be leftover cruft tomorrow because tomorrow we'll have a lot better tools to replace it. For example, that "dump_disks" function we talked about lately was just a crude approach to get any useful output, and yes, it was pretty much misplaced where it was at first, but it was there, it was available, and it served its purpose - until the next development step. We not can use much more useful YAML output instead because we have that nice new class that does that for us. This is what being agile is all about: Do things, improve them, iterate until the stakeholders are satisfied (which always means "good enough", never "perfect").
and use mechanisms that would be useful when that code is executed as an RSpec test or yast client.
q.e.d. CU -- Stefan Hundhammer <shundhammer@suse.de> YaST Developer SUSE Linux GmbH GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg) Maxfeldstr. 5, 90409 Nürnberg, Germany -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org