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: 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: 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:
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: We use custom `assert*` functions here and there. Remove them from backtrace.
Test Plan: Ran `XHPASTTreeTestCase`.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6411
Summary: Replace this old hard-coded implementation with the new vector-based, unicode-capable one.
Test Plan: Ran unit tests. Looked at revisions in Differential, using whitespace modes to bypass cache.
Reviewers: btrahan, Afaque_Hussain
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2379
Differential Revision: https://secure.phabricator.com/D6016
Summary: unlike the hooks I copy/pasted, these hooks have parameters
Test Plan: read it
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D5264
Summary: These hooks allow test cases to build shared resources -- notably, database fixtures.
Test Plan: See next diff.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D5258
Summary: We don't set $paths when running --everything.
Test Plan: Ran `arc unit --everything` with coverage.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D5260
Summary:
Some tests take longer (fixtures usually around 1 second for me) and also FB runs all tests on deploy.
I want to see all results immediately.
Test Plan:
Added `usleep(200000)` to `resultTest()`, then:
$ arc unit
Saw results printed one by one.
Also didn't pass `$renderer` to `ArcanistPhutilTestCase` and saw empty output.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5141
Summary:
This commit doesn't change license of any file. It just makes the license implicit (inherited from LICENSE file in the root directory).
We are removing the headers for these reasons:
- It wastes space in editors, less code is visible in editor upon opening a file.
- It brings noise to diff of the first change of any file every year.
- It confuses Git file copy detection when creating small files.
- We don't have an explicit license header in other files (JS, CSS, images, documentation).
- Using license header in every file is not obligatory: http://www.apache.org/dev/apply-license.html#new.
This change is approved by Alma Chao (Lead Open Source and IP Counsel at Facebook).
Test Plan: Verified that the license survived only in unit tests and LICENSE file.
Reviewers: epriestley, btrahan, edward
Reviewed By: epriestley
CC: aran, Korvin, davidrecordon
Maniphest Tasks: T2035
Differential Revision: https://secure.phabricator.com/D3881
Summary: Currently, a shutdown exception ("script exited with open transactions!") overwhelms the actual test failure exception, which is the one that needs to be fixed.
Test Plan: Ran a fixture test which opens a transaction and then throws. Got diagnostically useful output after this patch.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3780
Test Plan: Show Full Unit Results on this diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2777
Summary:
I just hope that we will not create `ArcanistPhutilTestCaseTestCaseTestCase`.
Also fix a copy/paste error (@edward's this time).
Test Plan: `arc unit`
Reviewers: epriestley
Reviewed By: epriestley
CC: edward, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2692
Summary:
`$this->getPaths()` gives us paths relative to repository root.
We resolve them relative to cwd.
Test Plan: [src] `arc unit difference`
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2634
Summary:
Move away from setModule(), to setPathPrefix(). Also simplify test location/selection.
Depends on D2621.
Test Plan: Ran "arc unit".
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2622
Summary: Mechanical changes from D2588. No "Class.php" moves yet because they aren't necessary for libraries to function.
Test Plan: See D2588.
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2589
Summary: Currently, if you change a symlink outside a libphutil library and the link target is something inside a libphutil library, we may enter an inifite loop in the "do { ... } while(...)" later. Just bail if the loop won't resolve.
Test Plan: Ran arc unit, Airtime reported the issue resolved by a similar fix.
Reviewers: cpiro, btrahan
Reviewed By: cpiro
CC: aran
Differential Revision: https://secure.phabricator.com/D2318
Summary:
Allow tests to be skipped by calling assertSkipped(). It's not really
an assertion of anything tangible; more like "assert that we can't
really assert anything right now".
Test Plan: Added a new test to the PhutilUnitTestEngineTestCase.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2312
Summary:
Don't use abstract subclasses of ArcanistPhutilTestCase, only use
concrete ones.
This lets you put common functionality in an abstract BaseTestCase
(which itself is a subclass of ArcanistPhutilTestCase), then implement
concrete subclasses of the BaseTestCase.
Test Plan: Tested with a simple Base -> {Case1, Case2} setup.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2300
Summary: When tests have a lot of output, show a diff of the expected/actual.
Test Plan: Used this when developing D2016 to examine large output usefully.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2017
Summary: Simpler assert function for asserting a type of exception was raised.
Test Plan: Wrote this for (and tested it with) D1836.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1837
Summary:
We currently raise a very confusing error when we hit this case:
Exception: The phutil library '' has not been loaded!
Because of the trickiness of init-order stuff, it's difficult to detect this more explicitly earlier -- instead, just raise a more descriptive error.
Test Plan: Ran "arc unit" in a copy of libphutil other than the one arc loads.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T580
Differential Revision: https://secure.phabricator.com/D1760
Summary:
Mark all applicable Arcanist classes as "final", except PhutilLintEngine, which
needs a little finesse.
@jungejason / @nh, does this break any Facebook stuff?
Test Plan: Linter no longer raises warnings. Ran "testEverythingImplemented" in
Phabricator.
Reviewers: btrahan, jungejason, nh
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1519
Summary:
- Show file/line so you can tell which assertion failed if there's a block
with a zillion of them and they don't have messages.
- Try to format stuff a little better.
Test Plan: - Ran some failing unit tests.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: https://secure.phabricator.com/D1304
Summary: If you modify a file at the root level (like the celerity map) we run
the dir/path checks in the wrong order and fail to detect that we can't possibly
find any modules. This leads to an infinite loop inside the while loop below.
Test Plan: Ran "arc unit" on a change which affects the celerity map.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: 1146
modules
Summary:
Right now, if you add a file to "__tests__/data/blah.testcase" it doesn't
trigger those __tests__, but it should. Trigger __tests__ in all parent modules
when a file changes.
(We could be a little more complex about choosing what to run but all the tests
are super fast so it hardly matters if we run "too many" tests.)
Test Plan: Ran "arc unit" on this, got a test trigger. Added a file to a
__tests__/data/ directory and got a test trigger. Added some var_dump() and
spot-checked things for sanity.
Reviewers: btrahan, jungejason, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 1127
Summary:
When a unit test throws an exception, provide more data (type, trace) in the
test failure message.
Previously, we would show only the message itself, which may not be very useful
in debugging test failures.
Test Plan: Ran "arc unit" on a test which throws, got a stack trace.
Reviewers: edward, btrahan, jungejason, nh, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, nh
Differential Revision: 1123
Summary:
Keeping unit tests speedy keeps them useful since people actually won't mind
running them. This diff records the time taken by each test and displays it nice
and colorized. Really, I just want to discourage non-unit tests from making
their way into ##__tests__##.
Some thoughts:
- The "acceptableness" times are subjective but if dependencies are properly
mocked the times seem to be ok. Integration tests that make network requests to
third-party endpoints and pull in megabytes of data will not survive. This is a
good thing.
- Fast tests get a gold star, encouraging small tests. I am sorry that the star
does not sparkle.
- There is no way for a programmer to admit that their test is going to be slow
in some cases. They will be shamed with red text for the life of their test.
- It might be confusing that fast but failing tests get green text and maybe a
gold star.
Test Plan: Ran some of the unit tests within Arcanist and libphutil. See
https://secure.phabricator.com/file/view/PHID-FILE-cdd3c94c219e0fd7470b/ for
sample output.
Reviewed By: epriestley
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: 588
Summary:
I'm making a move on Lisk stubbability and need these so I can set up
Lisk isolation unconditionally in Phabricator tests. Also document and organize
this class.
Test Plan:
Changed the expected counts in the meta-tests and got failures. Ran
all unit tests.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran
Differential Revision: 192
