Search Unity

Getting changes to our mainline source repository (aka trunk) has never been easy, but the problems scale with the amount of people trying to merge their work. We have grown quite dramatically over the last couple of years, and the work on new features, improvements, bug fixes, refactorings and, of course, automated tests is always in full swing. At any given time, we have hundreds of open Pull Requests against trunk.

As you might already know from our previous blog post on our QA process, we tend to work in separate branches, where all changes are subjected to an early in-branch testing. So what happens next? How do those changes actually get merged to trunk or other release branches?

image00

The first thing to do is open a new Pull Request (PR) in Ono (our source code management system, based on Kallithea), add a detailed description that contains the purpose, testing status, technical risk and any other relevant comments. Every PR needs to be reviewed by a peer who has domain-specific knowledge for each area it touches. They give feedback on the changes. Ono has a system that automatically suggests who should be invited to review certain parts of the code.

The code reviews are a critical part of our development process. The reviewers focus mainly on following questions:

  • Does the code achieve the intended purpose?
  • Does the code have good test coverage?
  • Does the code needs manual testing and has it been performed?
  • Is the code well-written and does it follow the Boy Scout Rule (“Always leave the campground cleaner than you found it.”)?
  • Does the code follow the coding standard?

The number of people involved in reviewing the PR may vary, depending on the amount of changes and areas being touched. It can get to as many as 10-15 people for bigger features. Based on this feedback, we edit the code until everybody agrees on the right approach. So we often add only team peers at first to gather the initial feedback and fix all found issues before involving a wider range of people. Also, we like to split PRs into smaller chunks with isolated changes, since those require fewer people to review them and the reviewers can look at the code more thoroughly.

When the final PR is ready and went through the last round of reviews, we run another round of tests. Usually, we run the set of build targets and automated tests suites that are most commonly/easily broken. We have special targets set up on our build automation server Katana to make this a one-click process for developers.

image02

For the final PRs, we also have to obtain an approval from one of the Release Managers.They assess the release risk and make sure all the right parties have discussed and seen the changes.

Once all the green dots have been collected and the tests have returned green, we can take the PR to the next step and add it to the Trunk Merge Queue. Voila! The PR is now waiting to be merged.

image05

It might seem that this is it, but it’s not the time to relax yet. Before being actually merged to trunk, the changes need to pass Trunk Queue Verification! We introduced this process to improve the stability of trunk. So what does that mean exactly?

Each batch of PRs is automatically merged on top of trunk in a test branch. Then it’s pushed to a draft repository. Then more builds are launched and we are run all tests. An automated system polls our build system, analyses build results and sends status messages to an internal Slack channel.

If a build or test fails, it will first be launched again, both on the same machine and on another one to see if it is a stable failure or an instability. A failure will also initiate a bisection: a build will be launched half the way to the last known working revision. Repeatedly starting builds in the middle of the untested range, the system finds the PR that introduced the failure and pings the owner in a Slack channel.

The offending PR is taken out of the batch. Then, the good part of the batch is automatically merged on top of the trunk and pushed. Owners are notified about this happy event by a ping in a Slack channel and the PR is closed.

Why do we go through this whole process? We aim to improve the quality and stability of Unity! Following these steps helps prevent bugs and test failures from reaching trunk.

How do you test code in your team? Let us know in the comments!

If you’re passionate about code quality and would love to contribute to making, testing or maintaining Unity, join us!

31 Comments

Subscribe to comments

Comments are closed.

  1. What’s this profiler problem in the Unity .(reducing FPS )

    https://www.youtube.com/watch?v=LO3iYXCoqlE&feature=youtu.be

  2. Christian Herzog

    October 18, 2016 at 5:06 pm

    Interesting insight. Since we are waiting on a fix which was “currently being worked on” last week, and a quick-fix was pushed months ago, how long do you typically have to wait for a bug fix to arrive in a patch release?
    For us, it’s around 6 months in total now for a show stopping internal bug.

    1. Aras Pranckevičius

      October 18, 2016 at 6:01 pm

      Usually anywhere between “one week” and “more weeks”. Do you have a case # for your bug?

      1. Christian Herzog

        October 19, 2016 at 7:57 am

        support.unity3d.com >> 62348 is the most current conversation.
        We also submitted a repro-project ( https://fogbugz.unity3d.com/default.asp?791434_oojboogr5kbe4llq ) + had some email conversation, which weirdly isn’t listed in the fogbugz list.

        1. Christian Herzog

          October 20, 2016 at 7:51 am

          Thanks, I’ll continue the conversation over there.

  3. Wowowowo… This sounds insanely over-complicated for all but the 10% of the “complicated” merges..

    How do you handle changes like “Fixed a typo in the comment.” Does this have to be manually approved by a release manager too?

    Of course not? So what about “Renamed and reordered some private variable declarations to be consistent with other parts of the class.”

    Or what about a checkin that reads “Added a bool to the function to suppress logging (for use in certain performance loops)”? Needs about 5 man-hours of peer review, checking and manager approvals?

    My point: Merges are always a grey zone. And this described procedure here sounds like you treating every bird poop on a window like the Mona Lisa..

    1. In practice a PR is never that small. If you’re making changes like fixing typos in comments, we have dedicated branches for that kind of change, and we batch them up into groups. For sure, that’s necessary to make the value of code review and build farm verification outweigh the overhead.

    2. Aras Pranckevičius

      October 18, 2016 at 6:00 pm

      What Richard said, overall. Most teams have “bugfix” or similar branches, where you just drop small changes/tweaks/fixes and forget about it. Then once a week or so one person takes that branch, does code reviews, runs tests and lands to “trunk”.

      And yes, everything is case by case basis. A pull request that adds comments or fixes typos is usually approved _really_ fast and goes through the whole thing really smoothly.

  4. To match a code style you could consider using a stylecheck hook in the CI build process.

    Your team is big, I collaborate in a team on opensource project that is way smaller but still its hard to manage the PRs. Stuff is sometime good on its own but too specific, other stuff is broad and too drastic, sometime you wait for stuff to be merged and later youll find out another thing has been refactored over time and you know it costs time to pick it up. Its tough, just wanted to say; keep up the good work!

  5. Have you ever thought on separate trunk into several modules so it doesn’t take as much time to get builds and merges become safer?

    I know getting a modular architecture working for the core of the engine it’s probably really complicated because everything is so tightly interconnected, but it could really improve the whole workflow on the long term.

    1. We’ve got an ongoing effort to get the engine split up into modules (and it’s been ongoing for a few years now, but we’ve got a lot of it done now). At the moment we still statically link all the modules together, but we’re getting ever closer to being able to build each module as a separate DLL.

      I don’t know if anyone has thought specifically about having a separate ‘trunk’ for each module – there are different problems that arise when you do that – but I do know that we’ve talked about things like “the changes in this PR are only in the AI module, so let’s only build the AI module, use all the other parts that were pre-built on trunk, and then only run the AI module tests.”

  6. Kudos. I just erase everything if merge to master fails.

  7. I’m so sory, repetition of the message is possible I write the first time.
    It is excellent that you tell community about quality your work, and this really big work, what are you doing, thank you for it!
    If it is possible, tell about optimization of a code, about “standard” of a code which at you is accepted in more detail, can be to eat need for a separate post on this subject? how you think?

  8. Robert Cummings

    October 17, 2016 at 6:08 pm

    Are there any downsides to this approach? Obviously there must be some, just interested in how these would be addressed in future.

  9. How long it takes to all tests finish running?

    1. The time can fluctuate based on the load on our build farm but on average I find an ABV (A Build Verification) which is the test we run on all PR’s to take about 5~ hours. We have many other tests, some that take a very long time which we run less often(some nightly, some weekly). Fortunately the test runner is clever so if a test fails we dont need to re run them all and can just run the failed test. We also sometimes request a PR run additional tests if the code touches a relevant area. Its all part of the review process.

      1. Cool! Thx for the answer.
        The automated bisection is a really clever idea!
        Yeah, being able to rerun the failed test is a real time saver.
        I worked on a project where a few tests could take 30+ minutes to run, each.
        If we wasn’t able to rerun only the failed tests the company would probably drop the idea of automated tests. When I left the company, we were with almost 200 tests. Small number but better than nothing.

  10. Very inspiring!

    What coding standard do you guys follow?

  11. Thanks a lot, nice post!

    Why exotic source management, not just Git?

    1. Aras Pranckevičius

      October 18, 2016 at 8:13 am

      Mercurial is so “dead” that companies like Google and Facebook use it for most of their code :)

      Seriously though, Na’Tosha wrote about a year ago about how/why we chose Mercurial http://natosha-bard.com/post/122632480712/mercurial-at-unity

      1. I also like Mercurial (as well as Git). But Elena mentioned “Ono (our source code management system, based on Kallithea)”.

        1. Ono and Kallithea are just web frontends/management systems for Mercurial.

        2. @RICHARD FINE Thanks for clarification!
          @ARAS PRANCKEVIČIUS Thanks for the link to another great post!

  12. Florian Noirbent

    October 17, 2016 at 3:42 pm

    How many developers are working at Unity Technologies now ?
    How did the merge process evolve in the time and with the team growing ?

    1. Aras Pranckevičius

      October 17, 2016 at 4:34 pm

      About 100-150 developers working on “the engine”, depending on how you count.

      The process has evolved multiple times. For example, around 2006, there was pretty much no process. No build process, besides “ hey can you build code on windows now please and send me the exe?” :)

      The automated queue verification / bisection process talked in this post was addd late 2015, and it’s awesome IMHO (it’s constantly being tweaked and improved).

      Code reviews we have started maybe in 2012 or so? I forget.

  13. I know this can be a little off topic, but would be possible share any news about visual scripting? It’s been on research for so long and we dont know nothing about… plus, I realy dont like to work with addons, so I’m waiting for a oficial solution ;D

  14. Marc-André Jutras

    October 17, 2016 at 2:25 pm

    Ah… A reason why nested prefabs will never happen, and was even dropped from the roadmap. Would require you to change deeply too many systems, and the review would be insane.

    1. Rune Skovbo Johansen

      October 17, 2016 at 2:35 pm

      Nested Prefabs is still on the roadmap.

      The PR process can be tough indeed on changes that touch a lot of systems, but we deal with that and have ways of lessening the blow by splitting such features up into smaller parts to an extent.