On 03/01/2016 10:01 AM, Josef Reidinger wrote:
On Mon, 29 Feb 2016 17:31:46 +0100 Stefan Hundhammer <shundhammer@suse.de> wrote:
On 29.02.2016 08:07, Ancor Gonzalez Sosa wrote: [...]
[...]
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.
We seem to disagree in the value of RSpec as examples substitute, but I think there is no way somebody can disagree in your reasoning about test coverage - you will always miss some test cases, even when the coverage tool claims a 100% coverage. It's crucial that you update your testsuite when you find a hole on it. So every bugfix should include the corresponding fix/addition to the testsuite.
I once read that ideal test coverage is 90%...if you have 99% you are too confident that it catch everything and if it is below, then it is too weak. I think it is reasonable rule as from my experience there is always some parts, where automatic tests are too complex and too fragile, that it is easier to test manually then always fixing automatic ones and such 90% advice is good start.
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)
[...]
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.
Ok. That's the goal, to discuss the approach. See below.
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.
I agree. I prefer otherway around. using IO streams which is more flexible and allow e.g. also streming for example. So something like
device_graph = DeviceGraph.deserialize(input_file) device_graph.serialize(output_file)
where you can have more flexible behavior like having input/output file also streams beside strings.
I hate when we concentrate on a side track of the example instead of the main point. ;-) The "not perfect" warning didn't help. I was not advocating for extracting the File.open call out of the method. Or about adding boilerplate code to handle files. My point was about the general approach of this # I find unnecessary and non obvious the need to fake hardware probing # just to work with a devicegraph that is described in a YAML file fake_probing = Yast::Storage::FakeProbing.new # An additional call (with responsibility on fake_probing) just to # create an empty devicegraph indexed in a global catalog devicegraph = fake_probing.devicegraph # devicegraph as an I/O argument Yast::Storage::FakeDeviceFactry.load_yaml_file(devicegraph,input_file) # I don't completely disagree, but I thing it's more natural in Ruby to # do it like it's done below Yast::Storage::YamlWriter.write(devicegraph, output_file) vs this (modified version to focus on approach instead of file writing) # I want to create a Devicegraph, so I instantiate a Devicegraph # factory, not a FakeProbing + DeviceFactory (notice the difference # between DevicegrahFactory and DeviceFactory) factory = Yast::Storage::DevicegraphFactory.new # It simply returns a meaningfull Devicegraph object. # No I/O argument. No catalog. No hardware probing fake. devicegraph = factory.create_from_file(input_file) # More natural for Ruby developers. Very opinionated, I admit devicegraph.serialize(output_file) I don't see why the second version is more focused on devicegraphs as an implementation detail than the first one. Can you elaborate on that? As I see it, in the first one you create an empty devicegraph and then populate it using a DeviceFactory. In the second one you create the devicegraph directly using a DevicegraphFactory. I don't see that as a problem or a more devicegraph-focused approach. Actually, as a user of that API I would expect that "behind the scenes" there will be also factories for each type of device, or at least a generic DeviceFactory (like we indeed have). But I would expect that DeviceFactory to return device objects, not to populate a given devicegraph.
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.
I agree that boilerplate should be reduced and ruby is quite good with it, so there is only a bit of syntactic sugar needed.
Already said. My example was (not obviously enough) not about adding the boilerplate File.open, but about the general design.
[...]
Cheers. -- Ancor González Sosa YaST Team at SUSE Linux GmbH -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org