Brew test-bot - Refactor cmd/brew-test-bot.rb

(Ladislas de Toldi) #1

Hi there! --

(cc @MikeMcQuaid @sjackman)

In my quest to getting brew test-bot working on Azure Pipelines for taps, I’ve been spending a lot of time navigating through brew-test-bot.rb (link).

It’s a very long file with 1802 loc, a lot of functions and lot of variables with similar names, which makes it king of hard to follow what’s really going on.

I’m far from being a Ruby expert, but I’m wondering if would be possible to separate the different classes in different files. It would make navigation much easier.

Also, for debugging purposes, I find myself adding a lot of puts to understand the logic of the process. It would be nice to add some --verbose debugging to follow along.

If it’s something you’d be interested in, I’d be more than happy to work on it and submit a PR on which we could improve.

Let me know what you think! :slight_smile:

Best,
-- Ladislas

(Steve Peters) #2

This was most recently discussed in the following thread:

(Mike McQuaid) #3

We’d welcome refactoring if it came along with tests simultaneously.

(Ladislas de Toldi) #4

Thanks @scpeters! My bad for not finding that thread by myself.

I’ve read the thread thoroughly and I share the same motivation as @iMichka. I also understand the concerns of @MikeMcQuaid and the necessity of providing tests simultaneously.

As previously said, I’m far from being a Ruby expert but I’d be more than happy to contribute to this project as much as I can.

Before starting, the first thing to do would be to define the general framework of the project:

  • understand what brew test-bot is currently doing, should/must do and should/must not do (@MikeMcQuaid)
  • specify precisely the intended workflow so that everyone can have the big picture and detailed view
  • share the needs, improvements, difficulties that Homebrew maintainers, Linuxbrew maintainer and taps maintainers have faced and the solutions they implemented or would like to see implemented (@iMichka & @all)

With that in mind, it will be much easier discuss and decide how we’ll proceed. From the top of my head, a to-be-completed list of things we will need to discuss and agree on before starting the work:

  • --options/flags to keep, add or remove
  • the general architecture
  • “one command to rule them all” or different commands with more specific actions that can be developed, tested, documented and improved without breaking the whole thing
  • CI platforms to support and their specific logics – something like a “CI Platform Abstraction Layer” (CIPAL) would help us keep the Travis/Circle/Jenkins/Azure specific logic out of the actual test, build and upload code. It would make it easier for users to add more CI providers as modules. The same can be done for Bintray/AWS/etc.
  • what to test, tools to write tests (specific framework)
  • a simple real-life program/formula that we can test against with all the use cases

@MikeMcQuaid should we keep the discussion in this thread? Move to Github? Open a new thread here just for that?

Let me know what you think.

Best,
– Ladislas

(Mike McQuaid) #5

Keeping discussion in this thread would be best.

Rather than figuring out what brew test-bot needs to do (or doesn’t) I’d suggest keeping the scope much smaller and figuring out what you want to do, why and reaching an agreement on the best way forward. I’m not going to pretend that brew test-bot is great code but, at this point, it is exceptionally well-tested and important code (through actual usage rather than automated testing) and care needs to be taken not to break it. Additionally, finding the code hard to read is also a product of what it is doing being non-trivial and it being long, new code as much as the code being badly written/structured (which it is).

A good starting point (if you were interested) from my perspective would be splitting classes and standalone functions into separate files/classes/modules which can have independent unit tests while preserving the current functionality. From there additional extraction of standalone functions and classes can be done and eventually consider splitting into separate commands (only “perform testing” and “perform upload” make sense to me for now).

(Steve Peters) #6

I think perhaps some documentation about what the code currently does would be one way to start. It’s a bit tricky because I often start by reading from the top of the file, but the real entry point is at def test-bot near the very bottom of the file. Then depending on ARGV, there are two different code paths to calling Test.new(), which leads further down the call stack. Perhaps we could work on a high-level overview of the flow of the current code?

1 Like
(Mike McQuaid) #7

I don’t think higher-level documentation of the flow will help/be a good idea if we’re planning refactoring anyway as it’s likely to change. I think restructuring the code so e.g. the initial cmd/test-bot.rb file just contains def test-bot which calls into other objects/functions would meet your goals without writing docs that will quickly become outdated.

(Steve Peters) #8

Do we want to copy the structure used by homebrew-bundle:

  • lib/bundle.rb mostly require statements pointing to files in subfolder
  • lib/bundle/* most of the actual logic
(Mike McQuaid) #9

Yes, that seems like a good call. I think the brew bundle code has managed to stay readable as it has grown.

(Mike McQuaid) #10

(it also has tests that cover 100% line coverage which would be an ambitious but great goal for brew test-bot).

(Steve Peters) #11

Ok, I’ve started the refactoring by splitting most of the logic out of cmd/brew-test-bot.rb and into lib/test-bot.rb in the following pull request.

1 Like
(Ladislas de Toldi) #12

Whaou! Thanks @scpeters for that!