Summary: Ref T5568. As discussed in IRC. This is very rough and not widely useable, but represents a solid first step.
Test Plan: Ran `arc unit` with a bunch of flags.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: rfreebern, aripringle, jaydiablo, BYK, tycho.tatitscheff, epriestley, Korvin
Maniphest Tasks: T5568
Differential Revision: https://secure.phabricator.com/D13579
Summary: Use `PhutilClassMapQuery` instead of `PhutilSymbolLoader`, mainly for consistency. Depends on D13572.
Test Plan: This hasn't been tested very comphrehensively as of yet.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13573
Summary:
Fixes T8912. Property `$project_root` was missing in `PytestTestEngine` class, resulting in
broken py.test wrapper. Also renaming the property so the linter is happy.
Test Plan: `arc unit --everything`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: kparal, epriestley, Korvin
Maniphest Tasks: T8912
Differential Revision: https://secure.phabricator.com/D13698
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: 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: 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:
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 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: 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: Ref T7977. The `ArcanistTestCase` class is pointless and can be replaced by `ArcanistPhutilTestCase`. Furthermore, it sorta makes sense to just rename `ArcanistPhutilTestCase` to `PhutilTestCase`. Depends on D12664 and D12666.
Test Plan: `arc unit`
Reviewers: avivey, #blessed_reviewers, epriestley
Reviewed By: avivey, #blessed_reviewers, epriestley
Subscribers: aurelijus, Korvin, epriestley
Maniphest Tasks: T7977
Differential Revision: https://secure.phabricator.com/D12665
Summary:
Ref T7977. The `PhutilTestCase::getLink` method currently relies on arcanist projects instead of repositories. Instead, make this logic a bit smarter by looking up the base URI from `phabricator.uri` (currently it is hardcoded to `https://secure.phabricator.com`).
Ideally, we would pass `?repositories=$REPOSITORY_PHID` to `DiffusionSymbolController` as well, but I don't know if this is worth pursuing.
Test Plan: This diff.
Reviewers: avivey, #blessed_reviewers, epriestley
Reviewed By: avivey, #blessed_reviewers, epriestley
Subscribers: aurelijus, Korvin, epriestley
Maniphest Tasks: T7977
Differential Revision: https://secure.phabricator.com/D12664
Summary: Ref T5655. After D9983, `ArcanistBaseWorkflow` and `ArcanistBaseUnitTestEngine` are deprecated.
Test Plan: Wait a sufficient amount of time before landing this.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D10004
Summary: There's no need for these properties to be (implicitly) public.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11276
Summary: This directory was missed, but should have been moved in D11202.
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11262
Summary: Ref T5655. `PHPUnitTestEngineTestCase` is the test case for `PhpunitTestEngine`.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: aurelijus, Korvin, epriestley
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D11203
Summary: Fix a variable name to be consistent with naming conventions.
Test Plan: `arc lint`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: aurelijus, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11140
Summary: This adjusts the behavior of PytestTestEngine so that it correctly reports test failures natively, rather than throwing a command error.
Test Plan: Yeah right
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10866
Summary:
Ref T5655. It is superfluous to include "base" in the name of an abstract base class. Furthermore, it is not done consistently within the code base.
In order to retain compatibility with external code, I have kept the `ArcanistBaseWorkflow` class (which trivially extends from `ArcanistWorkflow`), but it is now deprecated and should output a warning message. Similarly for `ArcanistBaseUnitTestEngine`.
Test Plan: Created a workflow which extends from `ArcanistBaseWorkflow`. Executed the workflow and saw a deprecation warning.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, aurelijus
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D9983
Summary: I'm pretty sure that `@group` annotations are useless now... I believe that they were originally used by Diviner?
Test Plan: Eye-balled it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, aurelijus
Differential Revision: https://secure.phabricator.com/D9855
Summary: Applied various linter fixes. Also make the `.editorconfig` file a bit more specific. Unfortunately, `arc lint --apply-patches` currently modifies some test data that it shouldn't, but this should be fixed after T5105.
Test Plan: Ran `arc unit` to make sure things weren't broken.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, aurelijus
Differential Revision: https://secure.phabricator.com/D9440
Summary: Ran `arc lint --apply-patches --everything` over rARC, mainly to change double quotes to single quotes where appropriate. These changes also validate that the `ArcanistXHPASTLinter::LINT_DOUBLE_QUOTE` rule is working as expected.
Test Plan: Eyeballed //most// of the diff.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, aurelijus
Differential Revision: https://secure.phabricator.com/D9269
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
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
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
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
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
Summary: This might not be universally desireable, but I found myself writing an additional linter (which I had called `WhitespaceTextLinter`) for the sake of these two linter tests. I figured it may be of use upstream, and so I decided to submit it as a diff. I won't be offended if it is rejected however.
Test Plan: `arc lint` and `arc unit` are both okay with it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7957
Summary: Currently, running `arc unit -- src/` returns with `No tests to run`, even if there are test classes in `src/__tests__/`. This diff changes this behaviour so that `arc unit -- src/` executes unit tests in `src/__tests__/`.
Test Plan: N/A. I suppose you could create a file `src/__tests__/SomeTestCase.php` and see for yourself.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7915
Summary:
Before, if PHPUnit crashed (syntax error, undefined constant, etc), you would get a relatively unhelpful error message:
firehed@Eric-Sterns-Mac-Pro ~/dev/php-lcd: arc unit
Exception
Clover coverage XML report file is empty, it probably means that phpunit failed to run tests. Try running arc unit with --trace option and then run generated phpunit command yourself, you might get the answer.
(Run with --trace for a full exception trace.)
This now checks the json and code coverage reports and tries to pull a useful error message:
firehed@Eric-Sterns-Mac-Pro ~/dev/php-lcd: arc unit
Exception
The test '/Users/firehed/dev/php-lcd/tests/PlateButtonsTest.php' crashed with the following output:
Fatal error: Undefined class constant 'EFT' in /Users/firehed/dev/php-lcd/tests/PlateButtonsTest.php on line 25
Call Stack:
0.0002 233104 1. {main}() /Users/firehed/dev/php-lcd/vendor/phpunit/phpunit/composer/bin/phpunit:0
0.0039 564872 2. PHPUnit_TextUI_Command::main() /Users/firehed/dev/php-lcd/vendor/phpunit/phpunit/composer/bin/phpunit:63
0.0039 565496 3. PHPUnit_TextUI_Command->run() /Users/firehed/dev/php-lcd/vendor/phpunit/phpunit/PHPUnit/TextUI/Command.php:129
0.0247 2280168 4. PHPUnit_TextUI_TestRunner->doRun() /Users/firehed/dev/php-lcd/vendor/phpunit/phpunit/PHPUnit/TextUI/Command.php:176
0.0293 2730760 5. PHPUnit_Framework_TestSuite->run() /Users/firehed/dev/php-lcd/vendor/phpunit/phpunit/PHPUnit/TextUI/TestRunner.php:349
0.1211 3996832 6. PHPUnit_Framework_TestSuite->runTest() /Users/firehed/dev/php-lcd/vendor/phpunit/phpunit/PHPUnit/Framework/TestSuite.php:745
0.1211 3996832 7. PHPUnit_Framework_TestCase->run() /Users/firehed/dev/php-lcd/vendor/phpunit/phpunit/PHPUnit/Framework/TestSuite.php:775
0.1211 3996832 8. PHPUnit_Framework_TestResult->run() /Users/firehed/dev/php-lcd/vendor/phpunit/phpunit/PHPUnit/Framework/TestCase.php:783
0.1233 3999752 9. PHPUnit_Framework_TestCase->runBare() /Users/firehed/dev/php-lcd/vendor/phpunit/phpunit/PHPUnit/Framework/TestResult.php:648
0.1236 4016432 10. PHPUnit_Framework_TestCase->runTest() /Users/firehed/dev/php-lcd/vendor/phpunit/phpunit/PHPUnit/Framework/TestCase.php:838
0.1237 4017240 11. ReflectionMethod->invokeArgs() /Users/firehed/dev/php-lcd/vendor/phpunit/phpunit/PHPUnit/Framework/TestCase.php:983
0.1237 4017520 12. Firehed\PlateButtonsTest->testSingleButtonOnButtonDown() /Users/firehed/dev/php-lcd/vendor/phpunit/phpunit/PHPUnit/Framework/TestCase.php:983
(Run with --trace for a full exception trace.)
Or if nothing was written to `stderr`, you may get something like this:
firehed@Eric-Sterns-Mac-Pro ~/dev/php-lcd: arc unit --no-coverage
Exception
Test Firehed\PlateButtonsTest::testSingleButtonOnButtonDown did not finish
(Run with --trace for a full exception trace.)
Test Plan:
`arc unit` for arcanist itself
`arc unit` before and after the change on a project where:
* all tests pass
* some tests are failing
* a test causes a fatal error (undefined constant, bad method call, etc)
* a test file is invalid (syntax error)
No regressions that I could find, and all crashes now display a more useful error.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: epriestley
CC: aurelijus, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7848
Summary: This updates the C# tools unit test engine (the derived version of xUnit.NET test engine that supports code coverage) to work after the updates in D7594.
Test Plan:
Switched the test engine from 'XUnitTestEngine' to 'CSharpToolsTestEngine' and ran `arc unit --everything`. The results were identical except that code coverage was provided.
Tested on Linux; will update or comment based on whether it works on Windows as well.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7640
Summary: This just removes the `print "Discovered test .."` code in the xUnit.NET unit test engine. If users need to diagnose what tests are being debugged, it can be found out with `--trace` and observing what build commands are invoked.
Test Plan: Ran `arc unit --everything` and didn't see the message any more.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7639
