Refactor for maintenance and testability

[marnen]

As mentioned in #24, the code in this gem is really hard to work with. I'm planning to refactor the gem to made the code more tractable, probably moving tests to RSpec in the process.

• [x] Translate tests to RSpec
• [x] Make a first refactoring pass to make better use of RSpec features
• [ ] Write better tests (many of the existing ones are just smoketests without really checking the functionality)
• [ ] Refactor as necessary

[lumean]

Sure, better tests are very welcome. I only did small modifications since I took over maintenance but agree the code is quite hard to maintain.shall I wait with the new release or release your two fixes already now and do another release after the refactoring?

[marnen]

@lumean I'd recommend releasing now. I don't know how long this refactoring will take.

[marnen]

I think I'm gonna set up CodeClimate and probably Travis on my fork too. @lumean You may want to set them up on the main gem repo, but that's up to you.

[lumean]

@marnen what would be your suggestion on the test aporoach?

I'm thinking since a while how I can automate tests that really prove the graphs look correct e.g. rendered in a browser. But so far I could't find an easy solution. Because in my opinion (the existing) low level unit tests that just verify if a certain svg tag is present are more wasting time than useful. This is like not trusting the xml library to produce correct output. In the end it can still be the case that there is a conceptual error and coordinates of a shape are completly wrong and this would slip the test. Or if you refactor internally something and use a different set of svg tag to achieve the same visual result this means also changing unit tests, again wasting time.

one approach i had in mind would be to use selenium to open the rendered graphs in a browser and do a screenshot and then compare pixel by pixel? against a "golden example". So this would be only a one time effort to create golden examples.

I'm really wondering how others are doing this, guess we are not the only on the world with that issue. What is your approach to such a situation?

This would be extremly helpful to verify nothing went wrong during refactoring.

[marnen]

@lumean I was planning to use Capybara and/or Nokogiri to verify that the SVG DOM has the elements in it that we expect it to have. If there’s a conceptual error, it will show up soon enough as a bug in the behavior of the gem, but there doesn’t seem to be a conceptual error so far.

I’m not too worried about what happens if we change the SVG output. At that point we’re changing the desired behavior of the gem, so the tests should change to reflect that.

Of course we could also compare bitmap renderings as you say, but that’s certainly not where I’d start, and I think I’d try to avoid that unless there’s no alternative. This gem generates SVG, not bitmaps, so IMHO its behavior is correct if the SVG is correct.

[marnen]

And to address your other point: no, testing the SVG DOM is not like not trusting the XML library to produce correct output. We’re not testing that the tags are well-formed; rather, we’re testing that we’re writing a combination of elements that makes sense. And for that, testing the DOM is absolutely the proper approach. We could also run the generated SVG through a validator service as part of the tests (I already know of some things that are invalid in the generated SVG, though probably every browser accepts them...)

[marnen]

Also: @lumean, what do you consider the most important things to verify? We can start there and figure out how to implement appropriate tests. I think probably the most important things to verify are that the SVG elements have appropriate shapes and attributes and that the SVG is valid.

[marnen]

Wow, there's a lot missing. I'm writing specs as I figure out the behavior of the classes from inspection, but there's an incredible amount to do.

One of the reasons I love test-first development is that this stage of trying to figure out what should have been tested, weeks or months later, doesn't really happen. :P

[marnen]

As I go forward, I'm trying to inspect the SVG files written as part of the test output to determine what the important characteristics of the graphs are and make them into RSpec tests. Since svg-graph2 already uses REXML, I've been trying to get by with that for my specs, but I'm probably going to introduce Capybara (even though that requires Nokogiri) in order to have access to nice things like CSS selectors and a better testing setup.

At some point, we might want to replace REXML with Nokogiri altogether in the implementation, for better performance and nicer API, but I'm not going to do that just yet, and certainly not as part of a testing improvement. :)

[lumean]

Yes, the testability was very very basic. I used mainly the few existing examples and verified them visually in a browser. But a lot of parameter combinations have probably never been tested and are broken.

That's why at some point personally I switched to C3js for graps and did a lightweight integration here.

[marnen]

Oh, yeah, I haven’t even looked at the C3 stuff beyond doing the most rudimentary translation into RSpec. I don’t understand what it does well enough to have an idea about testing it yet.

[marnen]

As for stuff being broken, perhaps I was just lucky that the line charts work pretty well. Basically I just wanted something simple that didn’t require client-side JavaScript and would work for the line charts I was doing, and I didn’t want to do the math from scratch if I could find a graph gem.

And for that, this gem does a pretty good job. I wonder, though, if I’m now doing more work in getting this gem to be maintainable than I would have if I’d just written a graphing library from scratch in the first place. :) It’s an interesting challenge, though, and I’ll keep working on it for as long as I feel like I can.

[lumean]

Yes agree with standard settings it works quite well. I had similar needs initially with no client side js. But it was more a wish from my side than a rwal requirement. If you start using all the details like changing font sizes and popups etc. you'll discover probably a lot of issues. Also in my previous company the need came for features like zoom etc.

Popups use js anyways. So in the end I thought why not switch completely to C3js which is a major project and well supported. For me was was not possible timewise to add all that functionality and even test it reasonably.

[marnen]

If you start using all the details like changing font sizes and popups etc. you'll discover probably a lot of issues.

That has mostly worked fine so far, with the exception of the rendering order issues with popups (which I plan to fix once I get this gem maintainable).

Popups use js anyways.

Yes, but the graph is still usable without them. I’d rather non-JS users degrade to a graph without popups than no graph at all.

Do have a look at http://covid-tracking-charts.herokuapp.com if you want to see how I’m using it.

[marnen]

@lumean It occurs to me that if examining the SVG DOM statically is not enough, we can have Capybara drive a browser engine, and then use it to inspect the computed properties on the elements we care about. This probably isn't a bad idea ultimately, though I'm not going to implement it till we have to. :)

[lumean]

@marnen I will need to read a bit about capybara, so far never worked with it. But sure, feel free to use whatever helps with testing or development. I appreciate all your inputs and the discussions a lot!

Was also thinking a bit more about the js thing. There are nice discussions about it also on stack exchange, seems the percentage of users with disabled js is around 1% however i also liked the idea of an svg without js dependency. Once I was researching if there is a pure ruby js engine that could for example render any js graphing library and then persist/extract the live DOM. But one one hand I couldn't find such a thing and on the other hand would probably be like using a sledgehammer to crack a nut... just to avoid writing and testing own code ;-)

[marnen]

I will need to read a bit about capybara, so far never worked with it.

It’s very useful for testing DOM interaction. It’s really meant for HTML and so there are a couple of things it doesn’t get quite right for XML, but it’s got a really good testing API and so far it’s a win for this gem.

seems the percentage of users with disabled js is around 1%

A lot of analytics platforms rely on JS, so I wonder how accurate this is. But 1% is still a lot of users in absolute numbers. Plus there are accessibility issues (which is why eventually I want to add ARIA annotations to the SVG...)

Once I was researching if there is a pure ruby js engine that could for example render any js graphing library and then persist/extract the live DOM.

Why pure Ruby? You could use something like PhantomJS for such a thing, but it seems silly when it’s not needed for a simple SVG graph.

I actually enjoy doing client-side JS development, but I don’t think it’s the right thing for every website. Google Docs needs it. A typical site like the one I’ve been using svg-graph2 on does not. People have forgotten how to do progressive enhancement and graceful degradation.

[marnen]

@lumean Anyway, I’ve been pushing my test code frequently so that you can get a sense of my approach. Let me know what you think.

[lumean]

@marnen I looked at your Rspec tests and like it a lot! Did I see correct that you didn't touch yet the library itself ? If you want I can also give you access to commit directly on this repo instead of the fork.

[marnen]

@lumean Correct! I’m uncomfortable touching the implementation code until there are tests that can verify that I haven’t broken anything, which is why I started doing this in the first place.

Commit access might be useful, but I do want to make sure I still give you the opportunity to review my pull requests.

BTW, if we do want to do visual testing, I just found out about percy.io.