Better pull requests process


#22

I would add QA resources during release, if my PR won’t be QAed for a week, and I have to build on top of that feature, I am going to pile up code (of course you could split the PRs, but if none will go through it’s not really a good workflow, as a change in the first one will trigger a change on any based on it etc).


#23

@oskarth

  • Remove manual QA as a blocker for PR merging

any reasons for this?

  • QA measuring ‘time to test/unblock’ as a target to optimize for

Please elaborate on this

  • QA testing nightly

We already do testing for nightly builds.


#24

@oskarth

  • QA testing nightly

Yes, we are already doing nightly testing, but in general, even we are hiding some new features behind the flag without manual testing they can cause (IMO):

  1. regression issues, especially in terms that we have 5 platforms
  2. when flag will be enabled and we will aim to test new features together, some unpredictable issues may appear and I doubt that it will be easy to differ what exactly PR caused them. A lot of issues are found during manual QA, so I can’t see any reason to have them in develop even behind a flag.
  • Don’t dedicate all QA to release

I agree that probably we can reduce amount of QA that dedicated to release testing, but it obviously means that release testing will take more time or will be less quality. So I doubt that it can be really helpful.


#25

just wanted to emphasize this.

Before we start discussing solutions it’s probably better to understand exactly what the pain points are from each role involved, for example we haven’t actually heard from QA what is painful from their perspective. We should be looking to relieve tension for everyone’s perspective. We should also be asking ourselves better questions.

To do that, it would be nice to get consensus on what are useful indicators that measure performance of what we’re trying to improve (from all roles). A better pull request and release process is actually a composite of satisfying a multitude of factors.

A good solution would be able to demonstrate a positive change on most if not all indicators. It will also make everyone more aware of the behaviour of the PR.

We kind of got this “big long-lived PR” as one of these indicators, and even that has been questioned. Also why are these PRs Big and Hard, is it only because of it’s code? does it change too many thngs to test, is it triggering multiple reviews/qa? is it actually being rebased ?
Are long-lived feature branches an issue if they merge well?

For example: Regressions / Commit-Revert Incidents per week might be another measurement?
Review length and frequency? QA length and frequency? Ratio of PR’s to QA’s. (not saying any of these are good just stimulating the mind)

We made a comparison with Embark in terms of how many PR’s it has open, having used Embark alot of the week, while an impressive project, it offsets QA onto developers and users, as a result I, as a user, cannot use embark test with a certain feature in the latest beta, where I can in stable. Having that said the best QA are honest-with-themselves developers and Embark identifies these bugs and fixes them far more rapidly than in status-react. I mention this not because I think either project is better managed, both have tradeoffs, I don’t think we’ve identified exactly what we are trying to solve, what we want.

Once we understand that, we can look at the behaviours that are causing those metrics to be too high, document why they are happening.

I would probably avoid pushing for any solutions at this stage, without first undestanding ane measuring our pain points, getting developer buy-in and clearing unresolved objections.


#26

added the link to our release process doc to the initial post


#27

Thanks Igor. Would be great to see the above documents from @anna @Anton @hester / design as well, as this is the best documentation we have for how we currently do things and why, from different points of view.


Follow up call

Set up a dedicated call to follow up on this thread with input from all stakeholders (dev, qa, design, product)

Attendance: If you are unable to/prefer not to attend, please try to ensure someone from your swarm/team can speak on your behalf, e.g. at least one from Core team, at least one from Design, etc. Please add relevant people to the call.

Before the call: Try to reflect about each other stakeholders POV (e.g.: devs want X, designers Y, we all want Z, etc) and, informed by that, come with some concrete ideas for improving things.

During the call: everyone will be given a chance to speak in order to give their perspective on things

Goals:

  • Understanding everyone’s pain points around PRs/Quality etc
  • primarily reach consensus on things we can do today to improve things.
  • Additionally, an understanding of what longer term improvements would look like and what the first step towards it is.

#28

Design process: https://notes.status.im/yuoZp0jRSz2fdGWkVW6bSg?view

@andmironov , @Maciej, @Denis: please add any pain points, you see as part of our release process and any comments in response to my thoughts below.

PR size

Big PR’s are not necessarily an issue for Design and actually have some benefits over small PR’s.

  1. When it concerns redesign of a feature (e.g. Wallet) there is lower risk of inconsistency in the UI when using a bigger PR. The smaller the PR, the higher the risk that some PR’s tied to an end-to-end flow make it through release while others don’t. One might include a reference to a settings screen while the PR that includes the actual Setting item does not make it. This results in a host of upgradability concerns in designing the UI on top of existing complexity, in turn leading to increased risk of dead ends and inconsistencies in end-to-end user flows.

Solution to create smaller PR’s is twofold:

  • Pure discipline; creating separate issues for anything that is out of scope of a given PR, but tied to the user flow. Including a timeline and upgrade path.

  • Hide under flag until related PR’s are approved; Where would it be possible to specify what related GitHub issues are? Can it be a precondition to have these specified to have a PR be approved?

  1. Another benefit of a big PR is that the responsible designer can more easily carve out time to take a detailed look to compare implementation to design. Often the designer is already working on a new project. All require problem solving, meaning they are depth first in another topic. Switching attention to review small PR’s frequently disturbs focus on a new problem and creates inefficiency in the design process.

Solution to create smaller PR’s is:

  • Allow for a dedicated time slot to review a bunch of smaller PR’s; consequence being that it might take a while for a designer to review as to not disrupt ongoing in-depth work on another project.

Benefit of small PR’s:

  1. Seeing more rapid improvement and is desirable for PR’s that affect a single component, screen or interaction with no ties to other parts of the app.

  2. Easier to assess what improvements are in the pipeline; more readable.

Design POV on PR process

  • Designers are often not invited as reviewer when a PR is created. Having someone of DEV, QA, Design should be common practice for PR’s that affect UI.

  • PR’s should be created after a sanity check by the developer. If implementation deviates from the design it’s more effective to directly contact the designer for additional specs, alternatives, assets, than to create the PR. Definition of Done for a PR is “Looks like the design”.

  • Figma uses virtual pixels, probably best is to agree on a scaling factor to address responsiveness @maciej, any other thoughts on this?

  • Understood that version control and before-after views of changes to a design can speed up not necessarily the PR process, but implementation in cases of changes after handoff. Looking at how to pull before - after updates and version info into GitHub with help of @Pedro. Meantime, version history (last update) can be found in Figma (Menu > File > Show Version History). Designers can name and describe changes there. @maciej @andmironov @denis Does this sound doable?


#29

Automated testing process https://notes.status.im/xPIDLnyQThycTMKFqI2tIA?view


#30

Manual QA process: https://notes.status.im/m8I37aiXTQS7LnMffta3gw?view


#31

today’s call agenda and meeting notes: https://notes.status.im/C5pj8g7gQOu9Wo8PtDZsMw?edit


#32

ADR: https://github.com/status-im/status-react/pull/7584 - let’s follow up in a few Core Dev calls to see if things are better, as well as to check in on progress of the two longer term projects. cc @cammellos @Anton

@volodymyr.kozieiev @rachel @anna thoughts on Desktop release? Not super prio, but perhaps we can take a nightly that seems Good Enough once we have mobile UI and call that the next alpha/beta release? This will ensure people don’t download nasty nightlies.


#33

@oskarth, totally agree that we can release new desktop as soon as mobile ui ready. I think we can even temporarily comment out wallet to reduce work surface and add it in the next release. Wdyt?


#34

Sounds good to me

Asdfaasf