Fail lint tests if no tests are found in the test directory

Summary: Ref T4570. Testing a directory with no recognized tests currently passes, but should fail.

Test Plan:
  - Ran `arc unit`.
  - Removed tests from a test directory, ran `arc unit`, got test failure.

Reviewers: leebyron, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4570

Differential Revision: https://secure.phabricator.com/D8434
Fail Arcanist tests when they make zero assertions

Summary:
Fixes T4570. When a test case doesn't make any assertions, fail it:

  - A tiny fraction of tests pass by not throwing. These tests can easily make a trivial assertion. It took about 5 minutes to fix them all (D8435, D8436).
  - In other cases, no assertions means a test construction problem, as with T4570. In these cases, failing loudly catches a severe error.
  - Fixes the no-assertion test cases in `arcanist/`
  - Makes the PHP 5.4 test pass for the moment, see discussion in T4334.

Test Plan: Ran `arc unit --everything`.

Reviewers: leebyron, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4570

Differential Revision: https://secure.phabricator.com/D8437
Fix unit test coverage for `NoseTestEngine`.

Summary:
I noticed that code coverage wasn't showing in Differential for
some repositories that we are using with Phabricator.

`arc unit` would should unit test coverage, but the paths were messy
(for example, `.//foo/bar.py` instead of `foo/bar.py`). As a result,
the code coverage info wasn't recognised as being for the correct
module.

I'm not sure why this logic is the way that it is... perhaps this is to
do with an older version of `nose` (I am using v1.3.0).

Test Plan:
I created a diff for an internal repository that we have, and
observed that code coverage was displayed in Differential.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran, seporaitis, avive, dctrwatson, zeeg

Differential Revision: https://secure.phabricator.com/D8433
Added some additional assertion methods.

Summary:
There are quite a few tests in Arcanist, libphutil and Phabricator that do something similar to `$this->assertEqual(false, ...)` or `$this->assertEqual(true, ...)`.

This is unnecessarily verbose and it would be cleaner if we had `assertFalse` and `assertTrue` methods.

Test Plan: I contemplated adding a unit test for the `getCallerInfo` method but wasn't sure if it was required / where it should live.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8460
Utilize `assertFalse` and `assertTrue` methods.

Summary:
Ref D8460.

Use `$this->assertFalse(...)` and `$this->assertTrue(...)` instead of `$this->assertEqual(false, ...)` and `$this->assertEqual(true, ...)` respectively.

Test Plan: `arc unit`

Reviewers: #blessed_reviewers, epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8461
Make assertTrue() / assertFalse() messages more descriptive

Summary: See discussion in D8460. Primarily, this prints out the failing value when a true/false assertion fails, so if it was something useful (like a function result) it's visible.

Test Plan:
Added `assertTrue("quack")`:

     FAIL  ArcanistDiffParserTestCase::testParser
  Assertion failed, expected 'true' (at ArcanistDiffParserTestCase.php:16).

  ACTUAL VALUE
  quack

Added `assertFalse("quack")`:

     FAIL  ArcanistDiffParserTestCase::testParser
  Assertion failed, expected 'false' (at ArcanistDiffParserTestCase.php:16).

  ACTUAL VALUE
  quack

Added `assertEqual("quack", "moo")`:

     FAIL  ArcanistDiffParserTestCase::testParser
  Assertion failed, expected values to be equal (at ArcanistDiffParserTestCase.php:16).
  Expected: quack
    Actual: moo

Reviewers: joshuaspence

Reviewed By: joshuaspence

CC: aran

Differential Revision: https://secure.phabricator.com/D8465
Ignore detached git branches.

Summary:
Fixes T4559.

It looks like the code currently (at least partially) handles this by checking for `(no branch)`. I suspect that the behaviour of `git` has changed (I am running version 1.9.0) because I haven't figured out what state to be in to cause `git` to output `(no branch)`.

Test Plan: Ran `arc branch` when on a detached branch.

Reviewers: #blessed_reviewers, epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4559

Differential Revision: https://secure.phabricator.com/D8466
Arcanist - add revision data to TYPE_LAND_WILLPUSHREVISION event

Summary:
pretty straight-forward stuff here. Note that in other events we use "fields" or "specification" rather than "revision"; I think "revision" is best particularly in this context where it is in fact the revision being landed.

Fixes T4565.

Test Plan: php -l

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: aran, epriestley, Korvin

Maniphest Tasks: T4565

Differential Revision: https://secure.phabricator.com/D8484
Lint engine seems to get null configuration manager in svn precommit hook workflow

Summary:
We recently tried to advance the arcanist HEAD in our release branch but failed, due to an exception in SVN pre-commit hook like this:

abort: Commit blocked by pre-commit hook (exit code 1) with output:
 LINT 1.3s FacebookWebJSLintLinter (1 file)
Exception
Some linters failed:
 - FacebookWebCopyrightLinter: BadMethodCallException: Call to a member function getConfigFromAnySource() on a non-object
(Run with --trace for a full exception trace.)

However `arc lint` works just fine. By searching the change history, it looks related to a few commits D7271, D7377, D7382, especially D7377, where configuration manager is added to the lint engine. Add it to the SVN precommit hook workflow too.

Test Plan: I am not quite sure how to test it out easily. Any suggestions?

Reviewers: lifeihuang, JoelB, epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: aran, epriestley, mikemag, Korvin

Differential Revision: https://secure.phabricator.com/D8492
Use hgsprintf() when managing bookmarks in arc patch in mercurial

Summary: Fixes T4596. I couldn't immediately reproduce this, but the `hgsprintf()` version is clearly more correct.

Test Plan: Used `arc patch --trace` to examine hg commands.

Reviewers: btrahan, durham

Reviewed By: durham

Subscribers: aran, epriestley

Maniphest Tasks: T4596

Differential Revision: https://secure.phabricator.com/D8512
Don't close revisions on "arc amend"

Summary:
Ref T4603. This workflow predates `arc land` and doesn't make much sense in modern Phabricator/Arcanist. It is surprising that `arc amend` will sometimes close accepted revisions, and we're better at detecting that repositories are tracked, and tracking repositories is easier.

Also fix some inaccuracies and old claims in the documenation and help.

Test Plan: Ran `arc amend`.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Maniphest Tasks: T4603

Differential Revision: https://secure.phabricator.com/D8522
Use more modern detection of repositories in "arc close-revision"

Summary:
Fixes T4603. We fire `arc close-revision --finalize` implicitly from `arc land`, which may close a corresponding Differential revision.

We want to close if the repository is not present in Phabricator (i.e., we'll never be able to close in response to the commit message, since we'll never see it). Historically, we used Arcanist Project -> "Tracked" to make this determination. Instead, just check if the working copy is associated with a repository. This is simpler, easier, and works better.

Test Plan: Ran `arc close-revision --finalize`.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Maniphest Tasks: T4603

Differential Revision: https://secure.phabricator.com/D8523
Support `git-format-patch` format in diff parser

Summary:
Fixes T4063. The `git format-patch` command produces a special header
and footer which we need to detect, strip, and parse.

Test Plan:
  - Added and ran unit tests.
  - Submitted a diff with `git format-patch HEAD^ --stdout | arc diff --raw`.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4063

Differential Revision: https://secure.phabricator.com/D8547
fix tab complete for out-of-workdir

Summary:
see https://github.com/facebook/phabricator/issues/546 - arc complete blows up when not
in a workdir.

There's no "is ArcanistWorkingCopyIdentity object valid" method, but getVCSType() looks like the
closest match.

git grep for `getProjectRoot` didn't reveal any more problmatic call sites.

Test Plan: `arc [tab] [tab]`

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8578
Allow arc to parse `svnlook` diffs from moves/copies

Summary: Ref T4697. This is similar to the existing stuff, but `svnlook diff ...` can also produce a "Copied" header. Currently, we choke on it in Herald when running pre-commit rules.

Test Plan: Added and ran tests. In the next diff, used this in a real system.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4697

Differential Revision: https://secure.phabricator.com/D8653
Strip comments during `arc diff --verbatim` workflow before submitting to Phabricator

Summary: Fixes T4649. The issue in that task is caused because we're submitting a block of text including comments. We've probably been doing this for a long time, but maybe were more liberal in parsing before. Instead, strip them.

Test Plan: Ran `arc diff --verbatim` and got "Dxxx" prefilled correctly.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4649

Differential Revision: https://secure.phabricator.com/D8658
Fix escaping of bookmarknames properly for `arc patch`

Summary:
Fixes T4596. I misunderstood this issue and D8512 was not correct. Specifically:

  - The `hg log` needs to be escaped, since otherwise "arcpatch-x" is interpreted as a revset.
  - The `hg update` does not need to be escaped, since updating to a revset doesn't make sense and the command never treats its argument as a revset.
  - The `hg bookmark` does not need to be escaped, for similar reasons.

Test Plan:
  - Ran these commands in isolation and got sensible, consistent results.
  - Ran `arc patch` several times in a row and got proper bookmark names.

Reviewers: btrahan, durham, rvanvelzen

Reviewed By: rvanvelzen

Subscribers: epriestley

Maniphest Tasks: T4596

Differential Revision: https://secure.phabricator.com/D8661
Add coverage reports to py.test runner

Summary: Of note this now forces the pytest-cov plugin to be installed unless they turn it off.

Test Plan:
```
../arcanist/bin/arc unit

COVERAGE REPORT
     97%     changes/models/test.py
    100%     tests/changes/api/serializer/models/test_testcase.py
    100%     changes/api/serializer/models/testcase.py
 UNIT OKAY  No unit test failures.
Updated an existing Differential revision:
        Revision URI: https://tails.corp.dropbox.com/D43387

Included changes:
  M       changes/api/serializer/models/testcase.py
  M       changes/models/test.py
  M       tests/changes/api/serializer/models/test_testcase.py
```

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8667
T4670 Remove the any-author flag from the which command, make any-author the default, and annotate the username of the owner of the revision.

Summary: Remove any-author flag from 'arc which' make any-author be the default behavior.  Annotate revision with the owners username.

Test Plan:
Apply a patch that you don't own (arc patch Dxxx) run 'arc which' verify that:
   1. You see the revision
   2. You see the original authors username next to the revision (owned by sjobs) etc.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8690
Make "arc land" support landing someone else's code that got there via arc patch

Summary: Ref T4670.

Test Plan: arc patch D8685; arc land --hold; verified i got a nice message asking me to be sure i wanted to land epriestley's code

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4670

Differential Revision: https://secure.phabricator.com/D8709