Summary: Improve `ArcanistClosingDeclarationParenthesesXHPASTLinterRule` (and rename it to `ArcanistDeclarationParenthesesXHPASTLinterRule`) to ensure that there is no whitespace before the opening parenthesis of a function/method declaration.
Test Plan: Added test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13675
Summary:
This is to fix `arc unit` when running a test file with no test results (e.g. skipped)
```
EXCEPTION: (RuntimeException) Undefined variable: last_test_finished at [<phutil>/src/error/PhutilErrorHandler.php:210]
arcanist(head=master, ref.master=d54cb072facd), deviantart(), phutil(head=master, ref.master=75f675747648)
#0 PhutilErrorHandler::handleError(integer, string, string, integer, array) called at [<arcanist>/src/unit/parser/ArcanistPhpunitTestResultParser.php:95]
#1 ArcanistPhpunitTestResultParser::parseTestResults(string, string) called at [<deviantart>/unit/DaUnitEngine.php:150]
#2 DaUnitEngine::parseTestResults(string, TempFile, string, string) called at [<deviantart>/unit/DaUnitEngine.php:82]
#3 DaUnitEngine::run() called at [<arcanist>/src/workflow/ArcanistUnitWorkflow.php:186]
#4 ArcanistUnitWorkflow::run() called at [<arcanist>/scripts/arcanist.php:382]
```
Test Plan: Create a test file with skipped tests. Run `arc unit`. Make sure the exception is not thrown.
Reviewers: joshuaspence, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, aurelijus
Differential Revision: https://secure.phabricator.com/D13640
Summary: Fixes T8805. Explicitly require int/float for duration. Adjust XUnit/Go parsers to provide one.
Test Plan: Ran unit tests.
Reviewers: btrahan, joshuaspence, chad
Reviewed By: chad
Subscribers: jsotuyod, champo, epriestley
Maniphest Tasks: T8805
Differential Revision: https://secure.phabricator.com/D13637
Summary:
Ref T8332. I find it really odd that I need to run `arc lint --everything --output xml > checkstyle.xml` and feel that it would be much more intuitive to just run `arc lint --everything --output xml` and have the output written to `checkstyle.xml`.
To provide context, we are running `arc lint --everything` in a CI job and parsing the results.
Test Plan: Ran `arc lint --everything --output xml` and saw the output written to file.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8332
Differential Revision: https://secure.phabricator.com/D13570
Summary: I think that this output was used during the early stage of `ArcanistConfigurationDrivenLintEngine`, but I question it's value nowadays. In particular, I find that this output makes the output of `arc lint --trace` significantly less useful.
Test Plan: Ran `./bin/arc lint --trace` and saw useful output.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13593
Summary: This host is no longer in service.
Test Plan: `git grep -i www.phabricator.com`
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D13586
Summary: Fixes T8714. When a test engine isn't returning the correct result type, shift suspicion onto it.
Test Plan: Faked error, got exception blaming test engine.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8714
Differential Revision: https://secure.phabricator.com/D13486
Summary:
Ref T8670. Ref T8657.
- When lint only has advice (no warnings/errors), consider it a "passing" build.
- Be a little louder about `sendmessage` calls failing because this stuff is not totally broken and that makes T8670-related things easier to catch/fix.
Test Plan: Created this revision.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8657, T8670
Differential Revision: https://secure.phabricator.com/D13436
Summary: Return `$this` from a bunch of setter methods for consistency. See also D13422.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13423
Summary: For consistency, a single space should separate `catch` and the following parenthetical expression.
Test Plan: Added test case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13389
Summary:
Ref T8095. Before uploading lint/unit results in the old way, try to ship them to autotargets.
If we can query and upload data to autotargets, do so, and then skip the older style uploads.
Test Plan: Used `arc diff` to ship data up via Harbormaster.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8095
Differential Revision: https://secure.phabricator.com/D13381
Summary: Add some tests to ensure that `ArcanistXHPASTLinterRule` subclasses are properly implemented. This should catch issues such as two linter rules having the same `ID` value. See D13272 for a similar change.
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13274
Summary: All base classes should extend from `Phobject` or some other classes. See D13275 for some explanation.
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13281
Summary: If I understand correctly, all classes should extend from `Phobject`?
Test Plan: This needs some further work
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13275
Summary: I found another issue with T8042... it seems that tests from the root directory (i.e. `src/__tests__` are not being discovered properly). The handling of this case (`$library_path` being the library root directory) can probably be improved, and I am open to suggestions. Depends on D13202.
Test Plan: Added to existing test cases.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13185
Summary: Minor tidying of documentation and adding some groups.
Test Plan: Eyeball it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13135
Summary:
Ref T8042. Tests were not being discovered in two different scenarios:
# Files modified at the same level as the library root directory.
# "Normal" directories like `src/unit/engine`.
This regression was caused by D12689.
Test Plan: Added unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8042
Differential Revision: https://secure.phabricator.com/D13126
Summary: Fixes T8374.
Test Plan:
```
$ arc diff
You have untracked files in this working copy.
Working copy: /Users/epriestley/dev/core/lib/arcanist/
Untracked changes in working copy:
(To ignore this change, add it to ".git/info/exclude".)
derp
Ignore this untracked file and continue? [y/N] ^C
```
Reviewers: btrahan, joshuaspence, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T8374
Differential Revision: https://secure.phabricator.com/D13096
Summary:
The `ArcanistXHPASTLinter` class is becoming quite bloated. This diff separates the class into one-class-per-rule, which makes everything much more modular. One downside to this decoupling is that code reuse between linter rules is much more difficult, although this only affects a very small number of linter rules.
There is still some further work that could be done here, but I defer this until a later diff:
- Rewrite `ArcanistPhutilXHPASTLinter` using `ArcanistXHPASTLinterRule`.
- Change the unit tests so that they are truly only testing a single linter rule.
- Maybe improve the way in which linter configuration options are handled.
- Make it easier to keep track of the linter rule IDs (see T6859).
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: johnny-bit, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10541
Summary: Fixes T8344. Prior to D12971, this always returned `array()`. It may now sometimes return `null`. Switch the behavior to be more similar to the old behavior.
Test Plan: Inspection.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T8344
Differential Revision: https://secure.phabricator.com/D13061
Summary: Fixes T8042. Changes the way that `PhutilUnitTestEngine` discovers unit tests. In particular, if you only modify a single test case then there is no reason to run all other test cases within the same directory (which is the current behavior).
Test Plan: Added some unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T8042
Differential Revision: https://secure.phabricator.com/D12689
Summary: Currently, arcanist error output is sent to `STDOUT` instead of `STDOUT`. This is annoying because I am running `arc lint --everything --never-apply-patches --output=xml > checkstyle.xml` and the `checkstyle.xml` file is not valid XML.
Test Plan: Forced a linter to throw an exception and ran `arc lint >/dev/null`... saw error output.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13043
Summary: Ref T8238. If a staging area is configured for a repository (see D13019), push a copy of changes to it after creating a diff.
Test Plan: Ran `arc diff` with various options, saw applicable changes get pushed to staging.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: cburroughs, epriestley
Maniphest Tasks: T8238
Differential Revision: https://secure.phabricator.com/D13020
Summary: Fixes T8259. Depends on D13016. Use modern logic to support large file uploads.
Test Plan:
- Diffed with some images, saw them show up in the diff.
- Diffed with a 12MB binary, saw it upload in chunks.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8259
Differential Revision: https://secure.phabricator.com/D13017
Summary:
Ref T8259. Currently, `arc upload` uses new logic but `arc diff` uses older logic internally. This prevents `arc diff` from uploading files larger than 4MB to newer servers.
Split the upload logic apart so the two upload pathways can share it. Callers now build a list of FileDataRefs and hand them to an Uploader for upload.
Test Plan:
Ran `arc upload` on:
- One file.
- Several files.
- Small files.
- Big files.
- Directories.
- Unreadable files.
- Files full of random data.
- The same file over and over again.
- The same big file over and over again.
- Artificially broke `file.allocate` and redid some of the simple cases (large/chunked aren't expected to work in this case).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T8259
Differential Revision: https://secure.phabricator.com/D13016
Summary:
Fixes T8314. Change the phutil_console_wrap() call to only have a single
string parameter.
Test Plan:
Ran `arc diff` added text into the title, summary and test plan fields,
but did not specify any reviewers. When prompted to continue, clicked 'No' and
saw the '(Message saved to commit message.)' string.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Maniphest Tasks: T8314
Differential Revision: https://secure.phabricator.com/D13015
Summary: Ref T7604. Remove "arcanist projects" from `ArcanistWorkingCopy` and a few other callsites. Depends on D12999.
Test Plan: Can't really think of how to test this.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Maniphest Tasks: T7604
Differential Revision: https://secure.phabricator.com/D12945
Summary: Ref T7604. Remove the `reenter_if_this_is_arcanist_or_libphutil` function. Some discussion in D12945.
Test Plan: Ran `./bin/arc --trace help` and saw things happen as expected.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7604
Differential Revision: https://secure.phabricator.com/D12999
Summary: Ref T7604. Remove call to the `arcanist.projectinfo` #conduit endpoint from `ArcanistWorkflow`. Depends on D12992.
Test Plan: Ran `arc which` and verified that repository information was present.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Maniphest Tasks: T7604
Differential Revision: https://secure.phabricator.com/D12971
Summary:
Ref T7604. Ref T8307. This was broken in D12962 because I only tested `arc patch --arcbundle`. Furthermore, this particular sanity check doesn't actually do anything now (see T8307).
Test Plan:
Ran `arc patch --nobranch D12971` successfully.
Auditors: epriestley
Summary:
Ref T7604. Remove a call to `arcanist.projectinfo` from `arc export`. This Conduit call was only used to detect the repository encoding, which we should be able to do locally anyway.
I was considering removing the `$projectName` from `ArcanistBundle` as well, but I'm not convinved that this is a good idea. Specifically, doing so would make it difficult to issue the "This patch is for the 'X' project, but the working copy belongs to the 'Y' project" error. We could potentially use the repository callsign for this purposes, but this isn't portable across installs.
Test Plan: I'm not quite sure how to test this. I suspect that this functionality isn't widely used anyway.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7604
Differential Revision: https://secure.phabricator.com/D12962
Summary: I found a few strings that I had missed, using a mostly-broken-but-somewhat-okay custom linter ruler (https://secure.phabricator.com/differential/diff/30988/).
Test Plan: Intense eyeballing.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: aurelijus, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12888
Summary:
Currently, a lot of `ArcanistExternalLinter` subclasses have something like this:
```lang=php
if ($err && !$messages) {
return false;
}
```
We can avoid this code duplication by moving this check to the `ArcanistExternalLinter` class.
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11321
