HomePhabricator

[doc] added documentation for pre-merge testing

Authored by kuhnel on Wed, Apr 21, 2:33 AM.

Description

[doc] added documentation for pre-merge testing

fixes https://github.com/google/llvm-premerge-checks/issues/275

Differential Revision: https://reviews.llvm.org/D100936

Event Timeline

jerryct added a subscriber: jerryct.Tue, May 4, 3:40 AM
jerryct added inline comments.
/llvm/docs/Phabricator.rst
166

Hi. I am not familiar with LLVM CI workflow so I like these changes but I got slightly confused.

Last week I read this: https://reviews.llvm.org/rG7f9717b922d4

It says

"""
This means that patches are built and tested after they are merged to the these
branches (aka. post-merge testing). This also means it's okay to break the build
occasionally, ....
"""

and I was under the impression that beside a human review there is no further interaction with a CI to merge a patch (which already made me wonder that there is really no CI for pre-merge).

Now this change says there is indeed a pre-merge CI workflow. Can you clarify

  • why is pre-merge testing described in Phabricator.rst and post-merge in DeveloperPolicy.rst?
  • isn't the statement above "patches are built and tested after they are merged" to hard in the context that there is also pre-merge testing?
  • are there conceptional differences between pre-merge vs. post-merge? For example:
    • pre-merge only tests one architecture vs. post-merge all architectures
    • pre-merge only tests unit tests (fast tests) vs. post-merge also test heavier tests (like sanitizer)
    • etc. Would it make sense to describe the differences pre- vs. post-merge?

If I totally missed the point ignore my comment--but I thought these changes are for people like me to get an overview while not that familiar with the LLVM setup and these are exactly my questions.

kuhnel added a comment.Tue, May 4, 4:30 AM

Added @goncharov as he's working on pre-merge testing.

Hi @jerryct ,

Yes you are right, things are not as clear as they should be. I guess the main reason is that the documentation does not always reflect the reality of the project. I have an improvement of the documentation as open issue. In an ideal world we would have a "test strategy" document, that would explain all of this.

Let me give you my high-level picture (not sure if everyone agrees with that):

  • Many LLVM processes and documentation are from a time when we were using the Subversion repo. So the CI setup is doing post-merge testing as pre-merge was quite hard to do in Subversion.
  • Today, we have two major CI systems doing post-merge: LLVM Buildbot (non-MacOS platforms) and Green Dragon (for MacOS). These have a long standing tradition and a clear commitment from the community to keep these builds green.
  • Post-mege is covering A LOT of different configurations and architectures, the build performance varies per machine and configuration. Reliability of these builds also varies.
  • Last March (or so) Mikhail and I launched pre-merge testing in Phabricator as a prototype. We wanted to demonstrate that it's possible and also to understand what it would feel like. So far pre-merge testing is optional as there is no commitment from the community (yet). Also we do have some unsolved issues (e.g. false positives on broken main branch, performance, platforms beyond Linux and Windows, long-term ownership).
  • The exception for pre-merge testing is libc++: This project wants to do 100% code reviews and wants all patches to go through pre-merge testing. However that's a local plan/agreement in that project.

I did not add pre-mege testing to the developer policy, as it's not (yet) an agreed services within the community.

Does that answer your questions?

Best,
Christian

Hi Christian,

Does that answer your questions?

yes, thanks.

Maybe it would be helpful to add one sentence which explicitly states that pre-merge is optional and one need to opt-in with what is described in "Requirements". Maybe in the chapter "Requirements".

Jerry