1
0
Fork 0
mirror of https://we.phorge.it/source/arcanist.git synced 2024-12-05 05:02:43 +01:00
Commit graph

2230 commits

Author SHA1 Message Date
Joshua Spence
88ff113f92 Add a LESS linter.
Summary: This class provides an adapter for [[https://github.com/less/less.js/ | lessc]].

Test Plan: Wrote and executed unit tests.

Reviewers: chad, #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8992
2014-05-06 06:50:44 -07:00
Joshua Spence
9a13c87ac4 Parse JSON as an associate array.
Summary:
It seems that in some situations, JSHint does not set the `evidence`
property. In such cases, PHP fails with `Undefined property:
stdClass::$evidence`. It would be safer to access the error object as an
associative array rather than as `stdClass`.

Test Plan: N/A

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8993
2014-05-05 21:18:43 -07:00
Joshua Spence
8523b98f39 Add a puppet-lint linter.
Summary: This linter is a wrapper around [[http://puppet-lint.com/ | puppet-lint[]].

Test Plan: Wrote an executed unit tests.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8990
2014-05-05 20:42:40 -07:00
Joshua Spence
48d52c30b0 Reduce method visibility.
Summary: These methods are declared `public`, but there are meant to be `protected` (as they are declared in `ArcanistExternalLinter`).

Test Plan: N/A

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8991
2014-05-05 20:31:33 -07:00
Joshua Spence
e00ce65200 Add an XML linter.
Summary: Add a linter which uses [[http://php.net/simplexml | SimpleXML]] to detect errors and potential problems in XML files.

Test Plan: Wrote and executed unit tests.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8989
2014-05-05 20:15:53 -07:00
Joshua Spence
ab5c1562c0 Add a JSON linter.
Summary: Provide bindings for [[https://github.com/zaach/jsonlint | JSONLint]], which is a useful tool for linting and validating JSON. Theoretically, this could be done with pure PHP, however it would not be trivial (`json_decode`, for example, does not provide any context as to JSON validation errors).

Test Plan: Wrote and executed unit tests.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8988
2014-05-05 20:15:35 -07:00
Andrew Jones
85000be96f Updating the pylint linter to set the output fomat correctly depending on the version of pylint installed
See: <https://github.com/facebook/arcanist/pull/161>

Reviewed by: epriestley
2014-05-05 19:05:16 -07:00
Burak Yigit Kaya
b4e91e1a37 Accept int as map key too
See: <https://github.com/facebook/arcanist/pull/163>

Reviewed by: epriestley
2014-05-05 19:01:51 -07:00
Joshua Spence
213997dc9d Fix flake8 unit test.
Summary:
This unit test is failing for me. It seems that I have a newer version of `flake8`. We should probably provide support for the most recent versions of any external tools, at least until we can implement version-specific stuff.

I am running `2.1.0 (pep8: 1.5.6, pyflakes: 0.8.1, mccabe: 0.2.1) CPython 2.7.6 on Linux`.

Test Plan: `arc unit`.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8987
2014-05-05 18:59:35 -07:00
Joshua Spence
c013124690 Convert more linters to modern formats with .arclint support
Summary:
Ref T3186. Ref T2039.

  - Convert JSHint to modern format; improve granularity of errors.
  - Convert PyFlakes to modern format;
  - Remove ApacheLicenseLinter and LicenseLinter (these have been deprecated for a very long time).

This is somewhat disruptive and will break some users by no longer respecting various path/config options. I'll sequence documentation and deprecation warnings in front of these.

Test Plan: Ran unit tests.

Reviewers: btrahan, #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin, joshuaspence, aran

Maniphest Tasks: T3186, T2039

Differential Revision: https://secure.phabricator.com/D6810
2014-05-05 18:58:13 -07:00
Joshua Spence
f2b341ae03 Add a getVersion function to ArcanistExternalLinter.
Summary:
This method will, theoretically, allow `arc lint` to be configured to require some minimum version of an external linter (although this would probably require significantly more work).

Additionally, the existence of this method simplifies the `getCacheVersion` function which, previously, was implemented by the external linters individually. Instead, a general approach to determining the version for cacheing purposes can be used.

Fixes T4954.

Test Plan: I'm not sure how to test this.

Reviewers: chad, epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Maniphest Tasks: T4954

Differential Revision: https://secure.phabricator.com/D8971
2014-05-05 15:10:20 -07:00
Joshua Spence
a7327ca0e9 Modernize ArcanistJSHintLinter.
Summary: Modernize `ArcanistJSHintLinter` by extending from `ArcanistExternalLinter` instead of `ArcanistLinter`.

Test Plan: Wrote and executed unit tests.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8965
2014-05-05 14:22:27 -07:00
Bob Trahan
96d3416218 Arcanst - add support for custom statuses to arc close workflow
Summary:
Fixes T4938. Now that we have customs statuses, this workflow needs to know about 'em.

One fun caveat is that Conduit isn't up and running to generate the help method, so I had to add a parameter to have the statuses list out. This could maybe be omitted since entering erroneous status options does indeed tell you the correct options.

Test Plan:
`arc close --list-status` -- got a list of statuses
`arc close T1` -- closed T1
`arc close T1` -- error T1 closed already
`arc close T1 -s foobar` -- T1 status set to foobar

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T1, T4938

Differential Revision: https://secure.phabricator.com/D8938
2014-05-01 16:14:03 -07:00
Peng Li
c4ec22b140 It should be also valid to have empty change under hg
Summary: Like git, it is also valid to have empty net change under hg. Should not throw if that is the case.

Test Plan: Tried on a hg repo.

Reviewers: lifeihuang, #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8888
2014-04-29 14:17:35 -07:00
epriestley
0df22a97a1 Add "CURLFile" to the class whitelist
Summary: See D8876, etc.

Test Plan: ~o.O~

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8877
2014-04-29 04:35:18 -07:00
Jinghao Yan
577914e623 Correct "agument" to "argument", not "augment"
See: <https://github.com/facebook/arcanist/pull/162>

It looks like the word `argument` appears many more times than `augment`, so I'm assuming `agument` is most likely to be a typo of `argument`.

Reviewed by: epriestley
2014-04-29 04:34:57 -07:00
Joshua Spence
30ecf46c11 Allow ArcanistExternalLinter flags to be specified as an array.
Summary: Personally, I prefer to specify command lines flags as an array rather than a string.

Test Plan: `arc unit`

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: aran, epriestley, Korvin, chad

Differential Revision: https://secure.phabricator.com/D8387
2014-04-23 16:22:49 -07:00
epriestley
6e597b292a Show build results in arc land
Summary:
Fixes T4809. When landing a revision, check for a (non-manual) buildable of the current diff. If we find one, check its status:

  - If it passed, print out a message to inform the user that we checked.
  - If it failed or is still building, print out details about the issue and require a confirmation to continue.
  - Just ignore other cases.

Test Plan:
  - Ran `arc land` on a revision with no buildable, a passing buildable, a failed buildable, and a building buildable for the current diff.
  - Got sensible output / prompts.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4809

Differential Revision: https://secure.phabricator.com/D8801
2014-04-17 15:59:54 -07:00
epriestley
23c9e87fdf After landing to a repository, hint that it should be updated
Summary: Fixes T4605. Smart waits (see D8782) are presumably good on the balance, but may cause some delays when changes are made to rarely updated repositories. To help mitigate this, have `arc land` hint that a repository has changed.

Test Plan: Will run `arc land`.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4605

Differential Revision: https://secure.phabricator.com/D8783
2014-04-16 13:01:14 -07:00
Bob Trahan
43792895a6 Arcanist - fix shouldAmend code
Summary: make it a real member variable. See rARC77a9c1814063 and D8753#33914.

Test Plan: sending up this diff!

Reviewers: shadowhand, epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8761
2014-04-11 13:07:54 -07:00
Bob Trahan
77a9c18140 Arcanist - normalize question about unstaged files across workflows (diff vs land)
Summary:
Fixes T4163. Re-word the question so its the same as in "arc diff". Draw the line though and don't automagically do anything if the user says "Y", 'cuz amending / adding files last minute in 'arc land' and other workflows is batshit insane. Assuming infinite growth in the future, I think its best to get this language consistent now.

I changed the shouldAmend member variable to a shouldAmend() function with a static inside. Previously shouldAmend was getting set as a side effect and it was kind of weird. I thought maybe it was written this way because the calls to the vcs are a little slow or something. As such, I figured caching it in the static was a good idea? Didn't seem awful but maybe a premature optimization with whatever the performance reality turns out to be.

Also a modest amount of bonus pht.

Test Plan: this very diff. i'm going to arc land it laters too.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: chad, epriestley, Korvin

Maniphest Tasks: T4163

Differential Revision: https://secure.phabricator.com/D8753
2014-04-11 10:52:35 -07:00
Bob Trahan
02456c0aed Arcanist - stop arc land if local is ahead of remote
Summary: Fixes T4291. Also pht user-facing strings.

Test Plan:
made a branch `foo` off master. made a commit and a diff in `foo`. switched backed to master and cowboy committed some thing. went back to branch`foo`, did an arc land, and saw the error message. went back to master, did a git resert --hard HEAD^1, went back to branch `foo`, and then successfully arc landed.

also ran arc help land and things looked good

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4291

Differential Revision: https://secure.phabricator.com/D8738
2014-04-09 12:35:31 -07:00
epriestley
ac528ab3a8 Changed the rendering of the priority column color based on the priorityColor property
Summary:
I changed the rendering of the bar color for the `priority` column when running `arc tasks` to match the `priorityColor` property.

If the `priorityColor` property is one of the basic colors already supported by ansi, then the bar is set to that color, otherwise it is set to white.

This will allow the user to customize maniphest priorities and then set their own colors and have those colors display correctly when running `arc tasks`

Fixed some linting errors

Test Plan:
Run `arc tasks` and ensure that:
 * the priority column is displayed with a colored bar
 * the priority column bar is the correct color (or white if it is an unsupported color)

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8735
2014-04-09 12:13:40 -07:00
Brayden Winterton
7e394dcf11 Changed checking for a closed task in arc tasks to look for the isClosed property instead of a non-empty status
Summary: I changed the way `arc tasks` was checking to see if a maniphest task was closed from looking for a non-empty status to looking for the isClosed property submitted in D8731.

Test Plan: Run `arc tasks` and check that the tasks show open/closed correctly.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8732
2014-04-09 07:48:58 -07:00
Bob Trahan
2714395d98 Arcanist - allow amending revisions by other authors in the working copy
Summary: Fixes T4670. Give the user an option to abort but otherwise proceed.

Test Plan: arc patch D8685 (by epriestley); arc amend

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4670

Differential Revision: https://secure.phabricator.com/D8716
2014-04-08 11:58:50 -07:00
Peng Li
0cff627d75 Fix an issue in arc patch with git-svn
Summary:
We recently moved our HEAD and it caused some issues on `arc patch` with git-svn repos. The base revision is incorrect and patch will fail. Add the check in such case to make it work.
The check was there before but removed in change b202158. The reason wasn't mentioned there though.

Test Plan: Tried it on svn.

Reviewers: lifeihuang, JoelB, #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8717
2014-04-08 10:46:27 -07:00
Bob Trahan
f099168aa8 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
2014-04-07 11:44:14 -07:00
Gabriel Guzman
0e5bea940c 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
2014-04-03 11:30:38 -07:00
David Cramer
b8e4261455 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
2014-04-02 11:31:38 -07:00
epriestley
11e2c1688f 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
2014-04-01 08:21:15 -07:00
epriestley
5280f3708e 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
2014-04-01 08:21:05 -07:00
epriestley
a36969e58b 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
2014-04-01 08:20:55 -07:00
Aviv Eyal
d0bab7f34a 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
2014-03-25 17:33:23 -07:00
epriestley
46fe94db9f 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
2014-03-15 11:25:49 -07:00
epriestley
1e6d958b27 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
2014-03-13 18:01:53 -07:00
epriestley
03ddc17032 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
2014-03-13 18:00:20 -07:00
epriestley
67239a08a5 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
2014-03-12 19:43:48 -07:00
Peng Li
ac82dea3c9 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
2014-03-11 15:35:28 -07:00
Bob Trahan
37dac61131 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
2014-03-11 15:21:46 -07:00
Joshua Spence
a2706f6539 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
2014-03-09 08:04:33 -07:00
epriestley
a90a72c648 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
2014-03-08 19:23:23 -08:00
epriestley
06cfe0746e 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
2014-03-08 18:33:05 -08:00
Joshua Spence
c42bef4e25 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
2014-03-08 18:23:14 -08:00
Joshua Spence
cf52d88f8b 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
2014-03-07 10:17:04 -08:00
epriestley
c6b1f3f070 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
2014-03-07 10:03:24 -08:00
epriestley
22f6207920 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
2014-03-07 10:03:17 -08:00
Peng Li
18251a1bd8 Remove a parameter
Summary: D7271 added this line but the signature of function ArcanistWorkingCopyIdentity::newFromRootAndConfigFile(...) was never changed. I have no idea why it was added here and it's causing failures in facebook. I am just removing it now.

Test Plan: No idea how to test

Reviewers: wez, lifeihuang, JoelB, #blessed_reviewers, epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8432
2014-03-06 15:53:06 -08:00
epriestley
133d289ad1 Generate a documentation book for Arcanist
Summary: Ref T988. Not pretty or polished, but works fine.

Test Plan: Generated the docs, made sure they were all there.

Reviewers: chad, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T988

Differential Revision: https://secure.phabricator.com/D8413
2014-03-05 13:00:40 -08:00
epriestley
7b6b24154b Add "Changes Planned" and "In Preparation" states for revisions
Summary:
Ref T2222. Discussion in T4481. Ref T2543.

Primarily, this will let me fix some of the rough edges that came out of ApplicationTransactions and state-based transitions.

I'm also adding a constant for T2543 while I'm in here.

Test Plan: Used `grep` to look for any weird special behavior, didn't see any.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2543, T2222

Differential Revision: https://secure.phabricator.com/D8400
2014-03-05 10:44:26 -08:00
Joshua Spence
fb826bbf9c Correctly identify @todo comments as TODOs.
Summary: Allow `@todo` comments to be linted as TODOs as well as `TODO` comments.

Test Plan: I added a new test case (`todo.lint-test`)

Reviewers: #blessed_reviewers, epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8389
2014-03-04 11:03:09 -08:00