Merge pull request #5084 from ethereum/updateContributing

[DOCS] Update contributing.

docs/contributing.rst

@@ -47,13 +47,14 @@ in addition to *what* you did (unless it is a tiny change).
 
 If you need to pull in any changes from ``develop`` after making your fork (for
 example, to resolve potential merge conflicts), please avoid using ``git merge``
-and instead, ``git rebase`` your branch.
+and instead, ``git rebase`` your branch. This will help us review your change
+more easily.
 
-Additionally, if you are writing a new feature, please ensure you write appropriate
+Additionally, if you are writing a new feature, please ensure you add appropriate
-Boost test cases and place them under ``test/``.
+test cases under ``test/`` (see below).
 
 However, if you are making a larger change, please consult with the `Solidity Development Gitter channel
-<https://gitter.im/ethereum/solidity-dev>`_ (different from the one mentioned above, this on is
+<https://gitter.im/ethereum/solidity-dev>`_ (different from the one mentioned above, this one is
 focused on compiler and language development instead of language use) first.
 
 New features and bugfixes should be added to the ``Changelog.md`` file: please
@@ -69,41 +70,47 @@ Thank you for your help!
 Running the compiler tests
 ==========================
 
-Solidity includes different types of tests. They are included in the application
+There is a script at ``scripts/tests.sh`` which executes most of the tests and
-called ``soltest``. Some of them require the ``cpp-ethereum`` client in testing mode,
+runs ``aleth`` automatically if it is in the path, but does not download it,
+so it most likely will not work right away. Please read on for the details.
+
+Solidity includes different types of tests. Most of them are bundled in the application
+called ``soltest``. Some of them require the ``aleth`` client in testing mode,
 some others require ``libz3`` to be installed.
 
-``soltest`` reads test contracts that are annotated with expected results
+To run a basic set of tests that neither require ``aleth`` nor ``libz3``, run
-stored in ``./test/libsolidity/syntaxTests``. In order for soltest to find these
+``./scripts/soltest.sh --no-ipc --no-smt``. This script will run ``build/test/soltest``
-tests the root test directory has to be specified using the ``--testpath`` command
+internally.
-line option, e.g. ``./build/test/soltest -- --testpath ./test``.
 
-To disable the z3 tests, use ``./build/test/soltest -- --no-smt --testpath ./test`` and
+The option ``--no-smt`` disables the tests that require ``libz3`` and
-to run a subset of the tests that do not require ``cpp-ethereum``, use
+``--no-ipc`` disables those that require ``aleth``.
-``./build/test/soltest -- --no-ipc --testpath ./test``.
 
-For all other tests, you need to install `cpp-ethereum <https://github.com/ethereum/cpp-ethereum/releases/download/solidityTester/eth>`_ and run it in testing mode: ``eth --test -d /tmp/testeth``.
+If you want to run the ipc tests (those test the semantics of the generated code),
+you need to install `aleth <https://github.com/ethereum/cpp-ethereum/releases/download/solidityTester/aleth_2018-06-20_artful>`_ and run it in testing mode: ``aleth --test -d /tmp/testeth`` (make sure to rename it).
 
-Then you run the actual tests: ``./build/test/soltest -- --ipcpath /tmp/testeth/geth.ipc --testpath ./test``.
+Then you run the actual tests: ``./scripts/soltest.sh --ipcpath /tmp/testeth/geth.ipc``.
 
 To run a subset of tests, filters can be used:
-``soltest -t TestSuite/TestName -- --ipcpath /tmp/testeth/geth.ipc --testpath ./test``,
+``./scripts/soltest.sh -t TestSuite/TestName --ipcpath /tmp/testeth/geth.ipc``,
 where ``TestName`` can be a wildcard ``*``.
 
-Alternatively, there is a testing script at ``scripts/tests.sh`` which executes all tests and runs
+The script ``scripts/tests.sh`` also runs commandline tests and compilation tests
-``cpp-ethereum`` automatically if it is in the path (but does not download it).
+in addition to those found in ``soltest``.
 
-Travis CI even runs some additional tests (including ``solc-js`` and testing third party Solidity frameworks) that require compiling the Emscripten target.
+The CI even runs some additional tests (including ``solc-js`` and testing third party Solidity frameworks) that require compiling the Emscripten target.
 
 .. note ::
 
-    While any version of ``cpp-ethereum`` should be usable, this cannot be guaranteed, and it is suggested to use the same version that is used by the Solidity continuous integration tests.
+    Some versions of ``aleth`` cannot be used for testing. We suggest using the same version that is used by the Solidity continuous integration tests.
-    Currently the CI uses ``d661ac4fec0aeffbedcdc195f67f5ded0c798278`` of ``cpp-ethereum``.
+    Currently the CI uses ``d661ac4fec0aeffbedcdc195f67f5ded0c798278`` of ``aleth``.
 
 Writing and running syntax tests
 --------------------------------
 
-As mentioned above, syntax tests are stored in individual contracts. These files must contain annotations, stating the expected result(s) of the respective test.
+Syntax tests check that the compiler generates the correct error messages for invalid code
+and properly accepts valid code.
+They are stored in individual files inside ``tests/libsolidity/syntaxTests``.
+These files must contain annotations, stating the expected result(s) of the respective test.
 The test suite will compile and check them against the given expectations.
 
 Example: ``./test/libsolidity/syntaxTests/double_stateVariable_declaration.sol``
@@ -115,10 +122,12 @@ Example: ``./test/libsolidity/syntaxTests/double_stateVariable_declaration.sol``
         uint128 variable;
     }
     // ----
-    // DeclarationError: Identifier already declared.
+    // DeclarationError: (36-52): Identifier already declared.
 
-A syntax test must contain at least the contract under test itself, followed by the separator ``----``. The additional comments above are used to describe the
+A syntax test must contain at least the contract under test itself, followed by the separator ``// ----``. The following comments are used to describe the
-expected compiler errors or warnings. This section can be empty in case that the contract should compile without any errors or warnings.
+expected compiler errors or warnings. The number range denotes the location in the source where the error occurred.
+In case the contract should compile without any errors or warning, the section after the separator has to be empty
+and the separator can be left out completely.
 
 In the above example, the state variable ``variable`` was declared twice, which is not allowed. This will result in a ``DeclarationError`` stating that the identifier was already declared.
 
@@ -131,7 +140,7 @@ editing of failing contracts using your preferred text editor. Let's try to brea
         uint256 variable;
     }
     // ----
-    // DeclarationError: Identifier already declared.
+    // DeclarationError: (36-52): Identifier already declared.
 
 Running ``./test/isoltest`` again will result in a test failure:
 
@@ -144,16 +153,16 @@ Running ``./test/isoltest`` again will result in a test failure:
             }
 
         Expected result:
-            DeclarationError: Identifier already declared.
+            DeclarationError: (36-52): Identifier already declared.
         Obtained result:
             Success
 
 
-which prints the expected result next to the obtained result, but also provides a way to change edit / update / skip the current contract or to even quit.
+``isoltest`` prints the expected result next to the obtained result, but also provides a way to change edit / update / skip the current contract or to even quit.
-``isoltest`` offers several options for failing tests:
+It offers several options for failing tests:
 
-- edit: ``isoltest`` will try to open the editor that was specified before using ``isoltest --editor /path/to/editor``. If no path was set, this will result in a runtime error. In case an editor was specified, this will open it such that the contract can be adjusted.
+- edit: ``isoltest`` tries to open the contract in an editor so you can adjust it. It either uses the editor given on the command line (as ``isoltest --editor /path/to/editor``), in the environment variable ``EDITOR`` or just ``/usr/bin/editor`` (in this order).
-- update: Updates the contract under test. This will either remove the annotation which contains the exception not met or will add missing expectations. The test will then be run again.
+- update: Updates the contract under test. This either removes the annotation which contains the exception not met or adds missing expectations. The test will then be run again.
 - skip: Skips the execution of this particular test.
 - quit: Quits ``isoltest``.
 
@@ -176,8 +185,9 @@ and re-run the test. It will now pass again:
 
 .. note::
 
-    Please choose a name for the contract file, that is self-explainatory in the sense of what is been tested, e.g. ``double_variable_declaration.sol``.
+    Please choose a name for the contract file that explains what it tests, e.g. ``double_variable_declaration.sol``.
-    Do not put more than one contract into a single file. ``isoltest`` is currently not able to recognize them individually.
+    Do not put more than one contract into a single file, unless you are testing inheritance or cross-contract calls.
+    Each file should test one aspect of your new feature.
 
 
 Running the Fuzzer via AFL
@@ -276,7 +286,7 @@ use the tool ``scripts/uniqueErrors.sh`` to filter out the unique errors.
 Whiskers
 ========
 
-*Whiskers* is a templating system similar to `Mustache <https://mustache.github.io>`_. It is used by the
+*Whiskers* is a string templating system similar to `Mustache <https://mustache.github.io>`_. It is used by the
 compiler in various places to aid readability, and thus maintainability and verifiability, of the code.
 
 The syntax comes with a substantial difference to Mustache: the template markers ``{{`` and ``}}`` are