(Everyday Code – instead of keeping our knowledge in a README.md let’s share it with the internet)
Today on a review I saw an RSpec Matcher that was more complex than it should be. We’ve discussed it internally. I think this article could be useful to our team and to the community as a whole.
An RSpec Matcher should be simple. It should return just ‘true’ or ‘false’
IRL example of a complex RSpec Matcher
This is the real live example of an RSpec Matcher from todays review
RSpec::Matchers.define :have_content_picture_type_set_to do |picture_ref, content_picture_type, content_object|
match do |page|
content_object_undersore = content_object.model_name.to_s.underscore
content_object_undersore = "tutorial" if content_object_undersore == "episode"
content_object_undersore = "courses/#{content_object.course.to_param}/section" if content_object_undersore == "course_section"
picture_ref_row = page.find(:xpath, "//tr[.//img[@src='#{picture_ref.content_picture.image_url}']]")
js_agnostic_click_table_action picture_ref.to_param, "/author/#{content_object_undersore}s/#{content_object.to_param}/pictures/#{picture_ref.to_param}/edit", "Edit Picture"
page.select content_picture_type, from: "content_picture_ref[content_picture_type]"
click_on_and_expect "Update Picture", "Picture successfully updated"
expect(page).to have_picture_ref_with_content_picture_type picture_ref, content_picture_type
end
failure_message do |page|
"expected page to have content_picture with the specified type, but was #{page.body}"
end
end
There is a lot to unpack here.
We do some calculations of paths with the whole content_object_underscore logic
Then we find something on the page with the page.find method
Then we call ‘js_agnostic_click_table_action‘ which is a complex method
Then we select something from an HTML select
Then we click a button with the click_on_and_expect
At the end we expect something
We should have only step 6. That’s the purpose of an RSpec Matcher. Return true or false if the page contains something or not.
Why has this happened?
Because RSpec.define :matcher is a function like any other ruby function. And as a ruby function you can do whatever you want with it. You can call anything you want in this function.
Why should an RSpec Matcher be simple?
That’s the convention for RSpec Matchers. Imagine the following spec:
visit_pictures material
expect(page).to have_picture_ref_with_content_picture_type material.content_picture_refs.first, "Thumbnail"
With this spec you would expect visit a page and check if there is something on the page. But now have_pictuer_ref_with_content_picture_type makes calls, changes the db, modifies the current page and there is no way for you to understand it from reading the spec. Let alone reuse the Matcher in another situation.
What feature is this about?
The feature above is about showing a picture on the animated 3D models and building instructions that we show at BuildIn3D. Authors could choose which picture is the Thumbnail – example is this FabBrix Hen :).
(Everyday Code – instead of keeping our knowledge in a README.md let’s share it with the internet)
I think calling a method thrее times is redundant. But then again, you have to balance. Today’s article is about a code review that at the end took like a few hours in total of different discussions and I believe it is important. This kind of things take time. Failed builds, difficult specs.
The IRL example – Where is the redundancy? Is this DRY?
This piece of code (along with a few other changes in the commit) were the root of a 2 hours discussion in the team. A spec failed because some things were int while we were expecting them to be float.
Calling ‘float’ three times like this is redundant
The reason I think like this is that if you have to change and would like to have a double value or an int value you would have to change the code in three places.
Probably e better solution would be:
+ if line_args[2] == "END":
+ rostep_params = {
+ "rot_x": LdrawLine.default_x,
+ "rot_y": LdrawLine.default_y,
+ "rot_z": LdrawLine.default_z,
+ "type": "ABS"
+ }
+ else:
+ rostep_params = {
+ "rot_x": line_args[2],
+ "rot_y": line_args[3],
+ "rot_z": line_args[4]
+ }
# We are adding a loop to call the float
+ for key in ["rot_x", "rot_y", "rot_z"]:
+ rotstep_params[key] = float(rotstep_params[key])
We call float only once at a single place. Now we have to deal with the fact that we have “rot_x” as a variable in two places, yes, that is true, but this could easily be extracted and we can iterate over the rostep_params values. But now we have consistency.
Is it harder to read? Probably it is a little harder. Instead of simple statements you now have a loop. So you lose something, but you gain something. A float function that is called at a single place.
What are we doing with these rotsteps?
ROTSTEP is a command in the LDR format. We support LDR for 3D building instructions. Here is one example with a FabBrix Monster that uses the LDR ROTSTEP as a command:
(Everyday Code – instead of keeping our knowledge in a README.md let’s share it with the internet)
We’ve used Rails after_action for controllers twice in our platforms and products. It allows you to execute something after a rails controller action.
Why? What is the IRL use case?
Users upload files for a 3D model or building instructions. We want to call an external service to convert this 3D model or buildin3d instructions and we schedule a job. We want to ping this external service. So we make a network request in the controller.
This is never a good idea. Generally you want to return fast from the call to your controller action. On Heroku you have a 15 seconds time limit to return and as a user experience it is good to return a result right the way and to do your job on the background and to report on the progress with some clever JS.
But for this specific case we have reasons to try to ping the external service. It generally takes about 0.5-1s, it is not a bottleneck for the user experience, and it works well. Except for the cases when it does not work well.
Today was such a day. Heroku dynos were trying to connect with the service, but it was taking longer than 15 seconds. I don’t know why. So i decided to revise the implementation.
What is special about after_action order?
It is the reverse of before_action.
We want to call the external service in an after action. In this way it is outside of the logic of the controller method and if it fails, we handle the exception in the after_action. It is a good separation. The real code is:
module IsBackend::JenkinsWorkoff extend ActiveSupport::Concern
included do
after_action do
unless @jenkins_rebuild == nil || @jenkins_rebuild.running?
begin
JenkinsClientFactory.force_jobs_workoff timeout: 10
rescue => e
config.logger.info e.message
end
end
end
end
end
The issue is with the order of the after_action when they are more then one.
Generally before_action are executed in the order they are defined.
before_action do
# will be first
end
before_action except: [:index, :destroy] do
# will be second
end
But after_action are executed in the reverse order
after_action do
# will be SECOND
end
after_action except: [:index, :destroy] do
# will be FIRST
end
When building the buildin3d instruction below we’ve pinged the external service in the build process. Because of the after_action we can enjoy this FabBrix dog.
(Everyday Code – instead of keeping our knowledge in a README.md let’s share it with the internet)
Today many things stopped in our dev procedures and I had to learn about heroku auth:whoami and to use it. Luckily, nothing about production, but a few engineers were puzzled as to why certain dev services are not working.
Heroku CLI logged itself out
We are using Heroku CLI a lot for a lot of different things. One example is backups. We backup the Heroku DBs regularly – like Daily, Weekly, Monthly and some of the backups stopped working. A few other services also stopped working. The problem was with Heroku CLI. The direct log from our Jenkins was:
heroku: Press any key to open up the browser to login or q to exit: ▸ Invalid credentials provided.
Enter your Heroku credentials:
Strange. It was working for years and now suddenly heroku cli decides to log itself out and it then requires us to log in. How come, we don’t know. I’ve created a Ticket with Heroku. My greatest concern is that it could be a security issue with someone gaining access to a dev machine and logging out, but I have no evidence of this.
$ heroku auth:whoami
I had to log in again with heroku. But I also wanted the builds that were ran with Jenkins to fail if heroku cli is not logged in. I found the command:
$ heroku auth:whoami
Which does the job. If we are not logged in the command will return and the bash script will fail.
(Everyday Code – instead of keeping our knowledge in a README.md let’s share it with the internet)
In Ruby String#sub! is different from String#gsub!. That much we know. But I am so used to .gsub that could not even see that the code in question uses .sub
I went over to change the regex because of a new requirement and I deployed and published on production.
But String#sub! replaces only the first occurrence of the regex, not all the occurrences. I could not event see that it was #sub! used and not #gsub!. Why would somebody one to replace only the first occurrence for this case – I don’t know.
(Everyday Code – instead of keeping our knowledge in a README.md let’s share it with the internet)
An ugly, sometimes over neglected truth of the industry is that the software that we work on does not become unmaintainable and tedious and difficult to work with overnight. It becomes like this with every change we introduce. While each individual change might not be “that bad” when they pile up we no longer have a well decoupled, maintainable system. We end up with a mess, which we than re-write with a new framework, a new team, new concepts and architecture just trying to do better. But the problem most of the time is in the single change that we do today – is it making the software system better or worse?
This article is about a practical example of today and how we got to stop it.
To better or worse
When developing software incrementally we introduce changes. There are basically two options – the new changes would make the system better or they will make it worse.
The logic is not important. We basically get the video for a material. What’s important is the code is practically the same in all 3 places. 4 calls in three files and they are all the same.
In the past I was leading a class in Software Development. One thing I tried to teach each student was:
When you copy and paste you introduce a bug. That’s a fact of the industry.
The reason is that once you copy/paste you would have to support the same logic in more than one place and you would simply forget about the second place the next time you would like to change the logic. It might not be you, it might be colleagues working years from now on the same code, but they will forget to change both places. That’s the problem with redundancy.
Remove the redundancy, and you remove most of the bugs
How did we stop it?
Simple review. Just ask a second person to check your commit. This simple review process protects you from a large portion of the other bugs that are still left after you’ve cleared all the redundancies (of course).
Enjoy
The code in question is part of the logic delivering these instructions. See how the video is displayed. That’s what the feature was about. Enjoy.
(Everyday Code – instead of keeping our knowledge in a README.md let’s share it with the internet)
Today I did some refactoring and I again worked with a query that I kind of never got to refactor and it is the most “complex” query that we seem to be comfortable with in our platforms because other queries are improved, but this one stays the same. This does not mean that it is not understandable or not maintainable, it is just the most “complex” we seem to be comfortable with, because any more complex than this and we naturally think about refactoring it.
This got me thinking.
What are other real life examples for the most complex relation database queries others are comfortable enough to live with before thinking about changing the model or the scheme.
Is my threshold too low or too high?
I would be happy to learn more from you.
Here is the query (Active Record) in question:
Get all the references for 'Tasks' that are in CourseSections that are in Courses
and filter all of them through the translations they have
These are 8 tables (ContentRef, Task, CourseSection, Course, and 4 translation tables)
# Get all the references for 'Tasks' that are in CourseSections that are in Courses
# and filter all of them through the translations they have
#
# These are 8 tables (ContentRef, Task, CourseSection, Course, and 4 translation tables)
ContentRef.
joins(:course_section).
includes(:translations).
includes(:task)
includes(task:[:translations]).
includes(course_section: [course: [:translations]]).
includes(course_section:[:translations]).
where(courses:{id: course_ids }, content_type: "Task").
select("content_refs.*, course_translations.*, course_section_translations.*, task_translations.*")
# The models is
Content Ref
belongs_to CourseSection
belongs_to Task
has_many :translations
CourseSection
belongs_to Course
has_many :translations
Course
has_many :translation
Task
has_many :translation
(Everyday Code – instead of keeping our knowledge in a README.md let’s share it with the internet)
In FLLCasts and BuildIn3D platforms we use three template languages. ERB (as it is all Rails), Liquid and Mustache. This article gives a brief overview of how we are using and extending Liquid in our platforms. The main purpose is to give an overview for newcomers to our team, but I hope the developers community could benefit from it as a whole.
What is Liquid?
Quoting directly from the site
Liquid is an open-source template language created by Shopify and written in Ruby. It is the backbone of Shopify themes and is used to load dynamic content on storefronts.
You create a template, you feed it values, it produces a result:
@template = Liquid::Template.parse("hi {{name}}") # Parses and compiles the template
@template.render('name' => 'tobi') # => "hi tobi"
Why Liquid?
Short answer – I thought: “It is good enough for Shopify so It should be useful for us”
With so many template engines it really is an interesting question why would we use Liquid.
When I made the decision years ago I was facing the following problem.
How to give admins the ability to create email templates where they write the id of an Episode. Before the email is sent, the template should evaluate what the title of this Episode is and fill it in the HTML.
The problem with Episode titles was the following. An author creates an Episode and sets the title to “Line following with LEGO Mindstorms”. An admin prepares a message for all users that the new “Line following with LEGO Mindstorms” tutorial is released. While the admins prepare the email and include the title of the tutorial in the Email, the title of the tutorial is changed to “Line following with LEGO Mindstorms EV3” by the author. So we end up with two titles of the tutorial. One in the email, and one on the site. This happens with Pictures also.
We are sending regular emails to subscribed users. As we are sending the emails I wanted to give the opportunity for admins to create a template of the email and then when the email is sent, the template is evaluated. The solution was to give admins the ability to create a “digest_message” in the platform and to fill this digest message with template values. Like for example:
Hello, {{ user.name }},
This is what we recommend you start with:
{% episode = Episode.find(1) %}
{{ episode.title }}
{{ episode.destroy SHOULD NOT BE ALLOWED}}
This was my goal. To get an instance of Episode, but without all the dangerous methods like “.destroy”. As admin I should still be able to call “episode.title”, but I should not be able to call “episode.destroy”.
This was how we started with Liquid. This was the first time we needed.
How do we use Liquid?
The code above is close to Liquid, but it is not Liquid. You can not use Episode.find(1) in Liquid and this is good. I don’t want this method to be available for admins to write in the emails. Because they could also write other, more dangerous methods.
Liquid gave us the solution – Liquid Drops
Liquid::Drop
The idea of Liquid::Drop is that you get a real object and you can call this object real methods while the template is evaluated. Here is our Episode drop
module LiquidDrops
class EpisodeDrop < Liquid::Drop
def initialize episode
@episode = episode
end
def title
@episode.title.try(:html_safe)
end
def description
@episode.description.try(:html_safe)
end
def published_at
@episode.published_at.strftime('%d %B %Y')
end
end
end
Here is how we use this drop in a Liquid Template.
As you can see we get an instance of episode 1189 and we can then call this instance some specific methods. Like episode.title or episode.description. This is not the real ActiveRecord object for Episode. This is a new instance that is wrapping the ActiveRecord object and is delegating specific methods to it. Liquid would only call methods that are available in the Drop.
The tricky part is to tell Liquid that it should connect ‘episodes.1189’ with the specific Drop class. We pass the drops as a context to template.render as:
@drops['episodes'] = LiquidDrops::EpisodesDrop.new
template = Liquid::Template.parse(template_content)
result = template.render(@drops)
In this way I’ve managed to solve our first challenge. Allow admins to write emails that can dynamically, when rendered, get the current name of the Episode without giving them access to more dangerous methods like Episode#destroy. I could have easily used ERB, but Liquid was such a good fit. It was designed exactly for this and Shopify were using it for their themes.
Screenshot and life example
If you register to FLLCasts (https://www.fllcasts.com) you will receive a couple of emails and all of them use Liquid.
If you don’t want to subscribe here is a screenshot from a recently sent email for a new Course.
Example from an email that was rendered with Liquid
You see the picture, title and description. These are all evaluated and added to the HTML with the use of Liquid.
Conclusion
This concludes the first part of this articles. In the next parts I will take a look at how we build Filters and Tags and how and why do we use them.
(Everyday Code – instead of keeping our knowledge in a README.md let’s share it with the internet)
We’ve designed, developed from scratch and are running an event-driven plugin based browser framework that we call Instructions Steps (IS). It helps us visualize 3D models and building instructions on the BuildIn3D and the FLLCasts platforms. Currently it consists of hundreds of extensions separated in about ~50 repos with 587 releases. We’ve figured out a way to keep the quality of the whole framework at a really good level with almost no bugs and errors.
This article is about how we are doing. The main purpose is to give an overview for newcomers to our team, but I hope the developers community could benefit from it as a whole.
The IS architecture (for context)
I will enter into the details about the architecture of IS in another article. For this article it is enough to say that IS consists of a really small core – 804 lines of code and a lot of extensions.
There are many extension that are extending the framework. Most of them are under 200 lines. The framework is highly decoupled and “everything is an extension”. Look at the 3D model and building instruction below. The “previous” button is an extension. The “next” button is an extension. You could have “parts list”, “bill of materials”, play animations, fit and rotate the camera. These are all extensions. I took this idea for the way we were building plugins for Eclipse (many years ago).
What is the problem with quality and how do we keep delivering a quality product?
Event-driven plugin based architectures have many advantages – like decoupling the plugins which makes them more maintainable. It forces you to have clear API boundaries which also makes them more maintainable. But there are a lot of questions and drawbacks compared to a nice Monolith app. What should we test? Should we test a specific extension, or the repo or the extension as it is working with all of its direct dependencies. What kind of specs should we develop? Should we have small unit spec that tests the extension in an isolation or we should put all the hundreds of extensions and test them all together. How do we make make these decisions?
Here are the few simple rules that we try to follow.
Compilation and type checking with Google Closure Compiler in ADVANCE mode
We use vanilla JavaScript. No TypeScript. There are reasons for this. Nothing against TypeScript actually. We use Google Closure Compiler to compile each and every extension.
Here is an example of a declaration of an “interface method”
/**
* Loads the given url and returns a Promise that when resolved will provide the caller with a {@link IS.StepsTree.StepData}.
*
* @export
* @param {string|File} file - url to the file or a DOM File object to be loaded
* @return {Promise} Promise that when resolved will provide a {@link IS.StepsTree.StepData} which is the root step
*/
IS.StepsTree.IProvider.prototype.getStepsTree = function(file) {};
Looking at the code we have the jsdoc annotations like “@param”, “@return”, “@export”. These are annotations that GCC understands and will check. It will check if the param is of the given type, it will check if the returned value is of the given type. It will check if the classes that implement this interface actually implement it.
Google Closure Compiler (GCC) will check if we are trying to access properties and methods that are not available.
As a general rule of thumb – compilers are strict. If they understand your code, and compile it, then your code fulfills a bare minimum of requirements.
Each extension is tested in isolation only with its direct dependencies available
The navigation extensions are located in the repo “is-navigation”. When we test the functionality of the “Next” button we don’t expect to also have the “Fullscreen” or the “Animations” extensions available.
Each extension is tested automatically in isolation, because each extension should work on its own given that it is the only extension that is installed (and the direct dependencies of course). Which makes sense. We are building a framework, a platform. When we have a framework, platform or even OS we should be able to install one extension, app, or program and they should be able to work on their own.
All extensions are tested together in the ‘release_pack’
What teams building platforms and frameworks quickly find out is that all the extensions and apps can work separately, but there are a lot of cases where if you put them all together and install them, things start to get more difficult. An example are all the different problems different OS have. One program is affecting another program in an unpredictable way.
So we’ve build the is-release_pack. What it does is to put all the extensions together and to run a few basic tests on all of them.
It contains 1-2 specs that check that each extension is working in the general case and probably one or two very specific cases. All the other specific cases are tested in the extensions, not in the “release_pack”. We push everything we can to the specs of the specific extension, but we have a few “integration” specs that are in the is-release_pack. And it is beautiful.
The downside of integration specs
There is one major downside with integration specs:
All of us, developers, are lazy when it comes to really building it right. Once we build the feature and we see that it is working after a day of work there is little motivation in us to spend the next 3 days on building it right. It just feels so go to have it working that you commit and move on to the next thing.
When there is a problem and an integration spec is failing most of the time it is easier to go and “ease” the integration spec. Change the expects a bit. Modify them. Even remove them.
Other times when we have to develop a specific spec for the specific extension it feels easier to develop an integration spec instead of 20 specific specs in the repo for the extension.
Sooner or later you end up in one of these situations:
There are no integration specs or they contain expects and assertions that can not validate that your product is working correctly.
There are a ton of integration specs that are constantly and randomly failing from time to time. The “integration” specs suite also takes forever to pass as there are now so many “integration specs”.
Both of this situations are highly undesirable.
Resolving the downside of integration specs
One thing I learn writing business plans when applying for different VC funding is RACI.
There are people Responsible for the job, people Accountable for the job, people that could be Consulted and people that should be kept Informed.
So who is Accountable for the delivery of the IS framework and for the framework working correctly with all extensions in the user browser?
Ideally it should be one person. In our case – it is Me.
We are all responsible for the implementation. But in a team one should be held Accountable if something is not working and not right. One is Accountable for not checking. This person could change of course, but at any given moment there is someone “starting the engine of the car” as it exists the factory. You should start the engine and make sure the car works. You are accountable for checking it. You might not be responsible if it does not start, but you are accountable for checking.
With the is-release_pack we resolved this for us.
Only the Accountable (Me in our case) has access to the is-release_pack and its specs. Nobody else. You can not add integration specs, you can not remove, you can not even change on your own. The person that is accountable should do it. We keep the number of specs to a mininum – one basic scenario for each extension and when appropriate 1-2 (but no more) very specific scenarios for each extension. In the release_pack we prefer to has scenarios that involve more than one extension. In fact if there is a scenario that involves all the extensions we would probably use it.
Integration specs are coupling the extensions?
Yes. They are. When one extension fails the integration spec for all the extensions fail. That is true. With hundreds of extensions if every day a different extension “fails” then you will not have a successful run of the integration suite in years.
But the customer “does not care”. The integration spec is the closes spec to the customer experience. The users never interacts with a single extension. They interact with all extensions.
In the same time if an extension has reached the release_pack and is failing the release pack , we will go and add a new spec, but not in the release_pack. We add it into the repo for the specific extension. This protects us from regressions.
Conclusion
By having a small subset of integration specs in a project to which only I have access and that is the final step in the release pipeline we’ve managed to stop hundreds of releases that would break existing clients, would lose a feature or introduce a bug.
587 official releases already and it takes 5 to 10 minutes to release the whole framework. Integration specs are present in the release_pack, but we keep them to a minimum, each testing many extensions at once and making sure that a real life client scenario is working.
Reply
You must be logged in to post a comment.