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

1985 commits

Author SHA1 Message Date
epriestley
3acbf9f3fa Expose --coverage and --no-coverage flags from arc diff
Summary: Currently, we don't expose these at top level, so you can't disable coverage if your coverage is explosively broken. Expose them as passthrough arguments.

Test Plan:

  - Touched a file in `arc` which triggered unit tests.
  - Without `xdebug` installed:
    - Ran `arc diff --preview`, `arc diff --preview --no-coverage` (both fine).
    - Ran `arc diff --preview --coverage`, got exception about coverage not being available.
  - Installed `xdebug`.
    - Ran `arc diff --preview`, got coverage.
    - Ran `arc diff --preview --coverage`, got coverage.
    - Ran `arc diff --preview --no-coverage`, no coverage.

Reviewers: indiefan, btrahan, vrana

Reviewed By: vrana

CC: aran

Differential Revision: https://secure.phabricator.com/D4745
2013-01-30 12:35:30 -08:00
vrana
9746d2b2a1 Use all changed files since base revision in repository cache key
Test Plan: Added debug output for repository version, linted, committed, linted again, saw the same version, changed file, saw different version.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4731
2013-01-30 11:25:16 -08:00
epriestley
51f32bdde7 Restore directory after executing ArcanistBundleTestCase
Summary:
This test currently chdir()'s into a directory which is later removed. If another test tries to run a shell script while the CWD is invalid, the shell may emit this to stderr:

  shell-init: error retrieving current directory: getcwd: cannot access parent directories: No such file or directory

Among other things, this can cause the XHPAST test to fail, because it detects syntax errors by examining stderr.

Instead, retore the directory.

Test Plan: Ran "arc unit --everything", which could previously fail if XHPAST ran after Bundle.

Reviewers: btrahan, vrana

Reviewed By: vrana

CC: aran

Differential Revision: https://secure.phabricator.com/D4738
2013-01-30 11:24:41 -08:00
vrana
c45053da4c Reset master after unsuccessful push
Test Plan: None.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4717
2013-01-29 12:12:31 -08:00
vrana
5aa3bc6ec0 Separate Phutil specific lint rules from XHPAST
Summary: Also provides an example how to build custom linter using XHPAST.

Test Plan: Added debug output to `willLintPaths()`, verified that each path is parsed only once.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4718
2013-01-29 10:54:40 -08:00
vrana
0586b12d2f Allow running arc land after git svn rebase
Summary: `git pull` may fail in git-svn after rebasing (which is a side effect of dcommit).

Test Plan:
  $ git svn rebase
  $ git log trunk..master
  $ git pull --ff-only; echo $?

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4716
2013-01-28 17:47:17 -08:00
epriestley
410b58a2e2 Correct a bunch of mercurial revset construction
Summary: Possibly improves T2387 state of the world?

Test Plan: not yet

Reviewers: bos, DurhamGoode

Reviewed By: DurhamGoode

CC: aran

Maniphest Tasks: T2387

Differential Revision: https://secure.phabricator.com/D4712
2013-01-28 16:28:14 -08:00
epriestley
2613ea196f Provide hgsprintf() for building hg revsets
Summary: I guess this is correct? See T2387 for discussion.

Test Plan: Unit tests.

Reviewers: bos, DurhamGoode

Reviewed By: DurhamGoode

CC: aran

Maniphest Tasks: T2387

Differential Revision: https://secure.phabricator.com/D4711
2013-01-28 16:27:47 -08:00
vrana
fd909479d0 Ignore untracked files in lint repository cache key
Summary: Some people have 2GB+ untracked files in repo which significantly slows down this or even crashes it.

Test Plan: Added a debug output here and linted repo with untracked path.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin, arudolph

Differential Revision: https://secure.phabricator.com/D4713
2013-01-28 15:40:47 -08:00
vrana
edd585a3d7 lint_untracked 2013-01-28 15:15:18 -08:00
epriestley
53691cdcf9 Fix an escaping issue with "svn commit"
Summary: Fixes T2438. We currently escape everything with '@', but SVN rejects that for '.'

Test Plan:
Unit tests. Performed this commit:

  $ svn st
   M      .
  A       x@123
  $ arc commit --conduit-uri=http://local.aphront.com:8080/ --revision 53

      Revision 'D53: asdf' has not been accepted. Commit this revision anyway?
      [y/N] y

  Committing 'D53: asdf'...
  Sending        .
  Adding         x@123
  Transmitting file data .
  Committed revision 37.
  Done.

I grepped for more '@' adding but couldn't find any. It's a bit tricky to grep for though, so it's possible I missed some.

Reviewers: btrahan, vrana

Reviewed By: btrahan

CC: aran, mbishopim3

Maniphest Tasks: T2438

Differential Revision: https://secure.phabricator.com/D4703
2013-01-28 14:11:31 -08:00
indiefan
ce0a491bcd Added base test result parser class. Implemented Go test result parser based on Go's built in test utility output.
Test Plan: Ran PhpunitTestEngine unit test and used both test result parsers to generate test results.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin, aurelijus

Differential Revision: https://secure.phabricator.com/D4676
2013-01-27 17:17:09 -08:00
Jesse Mullan
c49997a6e8 Added paths to search for python linters
Summary: Added paths to search for python linters

Test Plan: Linted in ubuntu

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2414

Differential Revision: https://secure.phabricator.com/D4648
2013-01-26 05:32:26 -08:00
Aurelijus
df04c79a90 Support PHPUnit 3.7 message changes
Summary: PHPUnit 3.7 now includes user message as well.

Test Plan: Ran phpunit tests

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4662
2013-01-25 17:19:41 -08:00
epriestley
44e25495ca Raise a lint warning on array_combine(x, x)
Summary: Suggest array_fuse().

Test Plan:
Unit tests, and:

  $ arc lint --lintall src/unit/engine/phutil/ArcanistPhutilTestCase.php
  >>> Lint for src/unit/engine/phutil/ArcanistPhutilTestCase.php:

     Warning  (XHP37) array_combine() Unreliable
      Prior to PHP 5.4, array_combine() fails when given empty arrays. Prefer
      to write array_combine(x, x) as array_fuse(x).

               252     $callable,
               253     $exception_class = 'Exception') {
               254     return $this->tryTestCases(
      >>>      255       array_combine(array_keys($map), array_keys($map)),
               256       array_values($map),
               257       $callable,
               258       $exception_class);

Reviewers: btrahan, vrana, chad

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D4659
2013-01-25 17:06:49 -08:00
indiefan
9946e23f9e Decoupling phpunit test parsing from running and gathering. Should enable third party test engines to reuse common business logic around phpunit without requiring common logic for the typically application-specific running and gathering.
Test Plan: Ran PhpunitTestEngine unit test. Also used refactored PhpunitTestEngine to run phpunit tests.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin, aurelijus

Differential Revision: https://secure.phabricator.com/D4651
2013-01-25 17:06:38 -08:00
durham
c8efb41811 Fix hg arc land on hg-svn repos
Summary:
arc land on a hg-svn repository would fail because arc land
uses 'hg push -r' to specify which revs to push which is not supported
by hg-svn. Now we just use 'hg push' which, when used against svn, only
pushes the current branch (which happens to be the branch we're trying to land).

We can't use standard 'hg push' for non-svn repos though because when used
against a vanilla hg repo 'hg push' pushes all branches.

Also remove --new-branch from 'hg push' because it's extremely
unlikely that a person wants to create a new branch on the server via
arc land.

Test Plan:
Ran arc land on a normal hg repo, verified it used 'hg push -r'.
Ran arc land on a hg-svn repo, verified it used 'hg push' and it pushed the
correct changes.

Reviewers: epriestley, sid0, dschleimer

Reviewed By: epriestley

CC: bos, aran, Korvin

Maniphest Tasks: T2403

Differential Revision: https://secure.phabricator.com/D4653
2013-01-25 14:00:52 -08:00
vrana
93ebde0d12 Revert "Delete useless code in ArcanistBaseWorkflow"
This reverts commit rARC501e2b7.
We call this method from FB workflow.
I'll commit it again in two weeks.
2013-01-24 16:33:32 -08:00
vrana
a1b902b190 Cache results of repository linters
Summary:
The cache key is repository base revision and hash of all modified files.
It isn't perfect in SVN where every file might have a different revision.
It's also suboptimal as just committing or amending changes the cache key even if the files contents weren't modified.
We can improve it later, perhaps by using previous revision and files modified since it.

Test Plan:
Changed granularity of XHPAST linter to repository.
Linted the same repo twice, verified that it was read from cache the second file.
Changed a single file, verified that all files were re-linted.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4608
2013-01-24 16:15:55 -08:00
vrana
e56f47aed5 Don't bail on no effect exception
Summary: The main reason for this is to not exit with 1 when no paths are lintable (which is more success than failure).

Test Plan:
Returned empty array from `buildLinters()`, then:

  $ arc lint
  $ echo $? # 0

Reviewers: epriestley

Reviewed By: epriestley

CC: wez, aran, Korvin

Differential Revision: https://secure.phabricator.com/D4625
2013-01-24 16:07:50 -08:00
epriestley
aec212069e Fix arc diff --no-ansi
Summary: Currently, we get an exception on empty %Ls for `arc diff --no-ansi` or similar (see P698).

Test Plan: Ran `arc diff --no-ansi`.

Reviewers: vrana, btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D4624
2013-01-24 14:28:28 -08:00
vrana
d3a6e456a6 Mark deleted paths as removed in committing
Test Plan: Deleted file in Git, ran `arc diff`, confirmed the question, saw the file as deleted.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4603
2013-01-23 14:42:49 -08:00
vrana
501e2b7e1d Delete useless code in ArcanistBaseWorkflow
Test Plan: This diff.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4601
2013-01-23 13:22:33 -08:00
Asher Baker
087ad5f84a Added support for hg diffs generated in git mode
Summary: Fixes T2112. These are fairly common now, and are used as the storage format for `hg export` and mq in most installs.

Test Plan: Ran unit tests, used `arc patch --patch`, uploaded some diffs manually.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2112

Differential Revision: https://secure.phabricator.com/D4592
2013-01-22 18:14:14 -08:00
epriestley
7f93b7de02 Don't use xhpast binary to compute xhpastlinter cache version if the binary doesn't exist
Summary: The binary may not be built, in which case this raises a warning.

Test Plan: Will make @zeeg test.

Reviewers: zeeg, vrana

Reviewed By: vrana

CC: aran

Differential Revision: https://secure.phabricator.com/D4569
2013-01-22 17:16:37 -08:00
durham
880964af83 Allow hg arc land when no rebase is necessary
Summary:
Previously, trying to arc land in a mercurial repo would
fail if the local branch was already at the tip of the onto branch
(since hg rebase exited with code 1).  This change makes it check
if a rebase is needed before executing the rebase.

Test Plan:
hg init foo
cd foo
hg bookmark master
touch a && hg add a && hg commit -ma
// setup your .arcconfig
cd ..
hg clone foo foo2
cd foo2
hg bookmark mybook
touch b && hg add b && hg commit -mb
arc land --onto master --revision <your rev number>

Arc land should succeed.  I also tried landing when a rebase was
necessary and it still worked.

Reviewers: epriestley, dschleimer, bos

Reviewed By: epriestley

CC: sid0, aran, Korvin

Differential Revision: https://secure.phabricator.com/D4588
2013-01-22 15:20:10 -08:00
vrana
98fec27752 Fix PHT linter
Summary: Variables in strings and concatenation lists.

Test Plan:
New unit tests.

  preg_match('/^((?>[^$\\\\]*)|\\\\.)*$/s', str_repeat('a', 1e6));

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4540
2013-01-19 11:48:17 -08:00
vrana
78017033bd Warn against usage of nowdoc
Summary: Also include binary hash in the version.

Test Plan: New unit test.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4535
2013-01-19 09:23:17 -08:00
vrana
85c0ed13d9 Set used variable 2013-01-18 13:32:48 -08:00
Nick Harper
40102252bf Fix arc diff when run from a git submodule
Summary:
A git submodule looks a lot like a normal git repo, but the .git
directory is replaced with a file that git reads to find the real
location of the git directory. When arcanist tries to write a file into
a directory inside of there, it was failing silently, and then crashing
silently when it couldn't read results back out. Instead of assuming the
git directory is a directory named .git at the toplevel of the tree, we
use the appropriate git command to get the correct git directory.

Test Plan: submit a diff from a submodule

Reviewers: epriestley, vrana

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4482
2013-01-17 11:28:13 -08:00
epriestley
a39b591c95 Allow pht(<<<HEREDOC ... HEREDOC)
Summary: Don't raise lint on heredoc literals.

Test Plan: Linted pht() + heredoc in D4477.

Reviewers: vrana, btrahan

Reviewed By: vrana

CC: aran

Differential Revision: https://secure.phabricator.com/D4478
2013-01-16 14:57:10 -08:00
vrana
90129c5432 Include real PEP8 version and options in lint cache key
Summary: This should be done for all external and configurable linters.

Test Plan: Linted file with lint problems, changed options, relinted.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4475
2013-01-16 12:17:11 -08:00
vrana
8142aab9d3 Fix whitespace around else 2013-01-16 12:15:46 -08:00
vrana
f25c01606b Cache stopped paths in lint
Summary:
If linter with file cache granularity stops linting then we don't run it on the same path next time.
But we still run linters with non-file cache granularity causing that they run even on the paths that would be stopped otherwise.

Test Plan:
Put global granularity linter after Generated linter.
Caused an error from this linter but in ignored path.
Verified that 'stopped' is saved in `lint-cache.json`.
Linted the same files second time, verified that the path is still skipped (wasn't before).

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4446
2013-01-15 17:58:32 -08:00
vrana
d4359a838a Use assert_instances_of(, 'array')
Summary: To have at least one real callsite.

Test Plan:
  $message = new ArcanistLintMessage();
  $message->setOtherLocations(array());
  $message->setOtherLocations(array(array()));
  $message->setOtherLocations(array(1));

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4419
2013-01-14 11:36:29 -08:00
vrana
e8decd2062 Display other locations of lint errors in console renderer
Test Plan: Created function named `f_a`, manually set other location.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4412
2013-01-11 15:25:58 -08:00
vrana
ff73a90482 Add hook after running linters
Summary: This resolves D4380#7 by reverting https://secure.phabricator.com/D4380?vs=9112&id=9123.

Test Plan: Used it in our linters.

Reviewers: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4409
2013-01-11 14:15:04 -08:00
vrana
5c5d347544 Format whitespace in XHPAST linter
Test Plan: Linted a method named `f_a`.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4405
2013-01-11 13:23:21 -08:00
vrana
a82493a84d Allow specifying multiple locations of the same lint error
Summary:
Some errors (duplicate declaration, invalid number of arguments) have more related places.
We need to notify user if he changes any related place.
This could be currently achieved by triggering errors instead of warnings or by including both files in the range (impossible if the locations are in different files) or by issuing multiple errors.
All options are too aggressive.

Test Plan: Issued error on unmodified line with other location on modified line.

Reviewers: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4392
2013-01-10 15:35:11 -08:00
vrana
80cd881fb1 Fix checking for lint binary paths
Summary: Like D4379.

Test Plan:
  $ arc unit

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4391
2013-01-10 15:34:42 -08:00
Gregor Stocks
767f9457c1 Add some new misspellings of "hierarchy"
Summary: turns out people really like misspelling "hierarchy"

Test Plan: hope
2013-01-10 14:32:23 -08:00
vrana
35de4a5d1f Run all willLintPaths() before any lintPath()
Summary:
FB runs some linters on background and it's a magnificent hack using `ParallelLinter` (two instances), `BackgroundLinter` and `FutureLinter`.
I want to simplify this by resolving the futures in engine instead of in some virtual linter.
It also seems like a better place to do it.
It should also fix caching problems I have with them (because the virtual linters don't know about the cache at all).

Test Plan: None yet.

Reviewers: epriestley, nh

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4380
2013-01-09 17:15:31 -08:00
epriestley
38e2e1336f Generate Git patches with "\n" metadata line endings unconditionally
Summary:
Fixes T2175. Git generates patches which have "\n" line endings on every system. We currently generate patches with system-dependent line endings.

Git accepts system-dependent line endings in almost call cases, but part of the parser tests for "\n" explicitly. T2175 has an example of this.

Test Plan:
Ran `arc export --git --revision D4366 > export.git` on a Windows machine, verified "\n" line endings. Ran the same with `--unified`, verified "\r\n" line endings.

(I didn't add any unit tests for this because it's Windows-dependent and very difficult to test meaningfully right now -- i.e., test that appliable patches are generated -- since the git reconstitution test doesn't run on Windows either, because we can't yet untar things there.)

Reviewers: btrahan, vrana

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2175

Differential Revision: https://secure.phabricator.com/D4373
2013-01-09 13:51:51 -08:00
vrana
983537e620 Fix checking for flake8 path
Test Plan:
  $ arc unit # without flake8 in path

Reviewers: zeeg

Reviewed By: zeeg

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D4379
2013-01-09 13:41:10 -08:00
epriestley
d399354822 Normalize Windows directory separators in SVN
Summary: Fixes T1946.

Test Plan: Ran `arc diff --only` with added paths inside directories. Saw only "/" entries in the diff.

Reviewers: btrahan, vrana

Reviewed By: vrana

CC: aran, mbishopim3

Maniphest Tasks: T1946

Differential Revision: https://secure.phabricator.com/D4374
2013-01-09 12:34:37 -08:00
epriestley
ed41f3f52c Allow arc.bat to work from paths with spaces in them
Summary: If you dump `arcanist/` into "Program Files" or similar, the `.bat` currently breaks. Quote the path so it doesn't. Fixes T1436.

Test Plan: Ran `arc` with `arcanist/` in a path with spaces in it.

Reviewers: vrana, btrahan

Reviewed By: vrana

CC: aran

Maniphest Tasks: T1436

Differential Revision: https://secure.phabricator.com/D4375
2013-01-09 12:34:30 -08:00
epriestley
57ec5a026d Fix arc diff x y in SVN
Summary:
D4186 added an "svn status --xml x y" form to getSVNStatus(), but the parser doesn't work for multiple files, since we get multiple <target /> elements in the XML output. So, curently, `arc diff` works (one target, all files) and `arc diff x` works (one target, x) but `arc diff x y` does not (more than one target, hits the exception).

  $ arc diff QUACK2 QUACK3
  Exception
  Expected exactly one XML status target.

Test Plan: Ran `arc diff QUACK2 QUACK3` in a working copy with modified QUACK2, QUACK3. Ran `arc diff`; `arc diff QUACK2`.

Reviewers: vrana, btrahan, codeblock, JThramer

Reviewed By: codeblock

CC: aran

Differential Revision: https://secure.phabricator.com/D4372
2013-01-09 09:11:26 -08:00
epriestley
ea1585d7fa Fix "empty diff" error in arcanist
Summary: Ref T2296. This error is unreachable right now -- when I fixed all the "\r\n" stuff, we always end up with a nonempty first line for an empty input. Do this test earlier and more explicitly. This results in a less useful error: "expected (some junk) on line 1" instead of "can't parse an empty diff".

Test Plan: Tried to parse an empty diff, got a "you can't parse an empty diff" error.

Reviewers: btrahan, vrana, codeblock

Reviewed By: codeblock

CC: aran

Maniphest Tasks: T2296

Differential Revision: https://secure.phabricator.com/D4370
2013-01-09 08:15:53 -08:00
Ricky Elrod
a5ddd7ebc0 Add a comma.
Summary: Add a comma because it was going to annoy the crap out of me.

Test Plan: See the comma. :)

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4365
2013-01-08 15:51:36 -08:00
Roy Williams
7547ef7e96 Pass the constructed message to the TYPE_DIFF_DIDBUILDMESSAGE event.
Summary: We want to use TYPE_DIFF_DIDBUILDMESSAGE to abort arc diff when a message doesn't fit some

Test Plan: Created an EventListener subscribed to TYPE_DIFF_DIDBUILDMESSAGE, validated the 'message' field was filled in

Reviewers: vrana, epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D4361
2013-01-08 15:06:46 -08:00