Add coding guidelines document

Started as a quite verbatim paste from:
https://wiki.fd.io/view/CSIT/Documentation#CSIT_Test_Code_Guidelines
then re-formatted, and new ideas added.

Change-Id: I13fc3d267f07d88cf22402051480613e55123d44
Signed-off-by: Vratko Polak <vrpolak@cisco.com>

1 files changed, 322 insertions, 0 deletions

diff --git a/docs/test_code_guidelines.rst b/docs/test_code_guidelines.rst
new file mode 100644
index 0000000000..51a99f077a
--- /dev/null
+++ b/docs/test_code_guidelines.rst
@@ -0,0 +1,322 @@
+CSIT Test Code Guidelines
+^^^^^^^^^^^^^^^^^^^^^^^^^
+
+The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
+"SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED",
+"MAY", and "OPTIONAL" in this document are to be interpreted as
+described in `BCP 14 <https://tools.ietf.org/html/bcp14>`_
+`[RFC2119] <https://tools.ietf.org/html/rfc2119>`_
+`[RFC8174] <https://tools.ietf.org/html/rfc8174>`_
+when, and only when, they appear in all capitals, as shown here.
+
+This document SHALL describe guidelines for writing reliable, maintainable,
+reusable and readable code for CSIT.
+
+TODO: Decide whether to use "you SHALL", "contributors SHALL",
+or "code SHALL be"; convert other forms to the chosen one.
+
+RobotFramework test case files and resource files
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
++ General
+
+  + Contributors SHOULD look at requirements.txt in root CSIT directory
+    for the currently used Robot Framework version.
+    Contributors SHOULD read `Robot Framework User Guide
+    <http://robotframework.org/robotframework/latest/RobotFrameworkUserGuide.html>`_
+    for more details.
+
+  + RobotFramework test case files and resource files
+    SHALL use special extension .robot
+
+  + Pipe and space separated file format (without trailing pipe
+    and without pipe aligning) SHALL be used.
+    Tabs are invisible characters, which are error prone.
+    4-spaces separation is prone to accidental double space
+    acting as a separator.
+
+  + Files SHALL be encoded in UTF-8 (the default Robot source file encoding).
+    Usage of non-ASCII characters SHOULD be avoided if possible.
+    It is RECOMMENDED to `escape
+    <http://robotframework.org/robotframework/latest/RobotFrameworkUserGuide.html#escaping>`_
+    non-ASCII characters.
+
+  + Line length SHALL be limited to 80 characters.
+
+  + There SHALL be licence text (FIXME: add link) present
+    at the beginning of each file.
+
+  + Copy-pasting of the code NOT RECOMMENDED practice, any code that could be
+    re-used SHOULD be put into a library (Robot resource, Python library, ...).
+
++ Test cases
+
+  + It is RECOMMENDED to use data-driven test case definitions
+    anytime suite contains test cases similar in structure.
+    Typically, a suite SHOULD define a Template keyword, and test cases
+    SHOULD only specify tags and argument values::
+
+        *** Settings ***
+        | Test Template | Local Template
+        ...
+
+        *** Test Cases ***
+        | tc01-64B-1c-eth-l2patch-mrr
+        | | [Tags] | 64B | 1C
+        | | framesize=${64} | phy_cores=${1}
+
+  + Test case templates (or testcases) SHALL be written in Behavior-driven style
+    i.e. in readable English, so that even non-technical project stakeholders
+    can understand it::
+
+        *** Keywords ***
+        | Local Template
+        | | [Documentation]
+        | | ... | [Cfg] DUT runs L2 patch config with ${phy_cores} phy
+        | | ... | core(s).
+        | | ... | [Ver] Measure MaxReceivedRate for ${framesize}B frames\
+        | | ... | using single trial throughput test.
+        | | ...
+        | | ... | *Arguments:*
+        | | ... | - framesize - Framesize in Bytes in integer\
+        | | ... |   or string (IMIX_v4_1). Type: integer, string
+        | | ... | - phy_cores - Number of physical cores. Type: integer
+        | | ... | - rxq - Number of RX queues, default value: ${None}.\
+        | | ... |   Type: integer
+        | | ...
+        | | [Arguments] | ${framesize} | ${phy_cores} | ${rxq}=${None}
+        | | ...
+        | | Given Add worker threads and rxqueues to all DUTs
+        | | ... | ${phy_cores} | ${rxq}
+        | | And Add PCI devices to all DUTs
+        | | ${max_rate} | ${jumbo} = | Run Keyword
+        | | ... | Get Max Rate And Jumbo And Handle Multi Seg
+        | | ... | ${s_24.5G} | ${framesize} | pps_limit=${s_18.75Mpps}
+        | | And Apply startup configuration on all VPP DUTs
+        | | When Initialize L2 patch
+        | | Then Traffic should pass with maximum rate
+        | | ... | ${max_rate}pps | ${framesize} | ${traffic_profile}
+
+  + Every suite and test case template (or testcase)
+    SHALL contain short documentation.
+    Generated CSIT web pages display the documentation.
+    For an example generated page, see:
+    https://docs.fd.io/csit/rls1807/doc/tests.vpp.perf.tcp.html
+
+  + You SHOULD NOT use hard-coded constants.
+    It is RECOMMENDED to use the variable table
+    (\*\*\*Variables\*\*\*) to define test case specific values.
+    You SHALL use the assignment sign = after the variable name
+    to make assigning variables slightly more explicit::
+
+        *** Variables ***
+        | ${traffic_profile}= | trex-sl-2n-ethip4-ip4src254
+
+  + Common test case specific settings of the test environment SHALL be done
+    in Test Setup keyword defined in the Setting table.
+
+    + Run Keywords construction is RECOMMENDED if it is more readable
+      than a keyword.
+
+    + Separate keyword is RECOMMENDED if the construction is less readable.
+
+  + Post-test cleaning and processing actions SHALL be done in Test Teardown
+    part of the Setting table (e.g. download statistics from VPP nodes).
+    This part is executed even if the test case has failed. On the other hand
+    it is possible to disable the tear-down from command line, thus leaving
+    the system in “broken” state for investigation.
+
+  + Every testcase SHALL be correctly tagged. List of defined tags is in
+    csit/docs/tag_documentation.rst (FIXME: rst-ize the link) file.
+
+    + Whenever possible, common tags SHALL be set using Force Tags
+      in Settings table.
+
+  + User high-level keywords specific for the particular test suite
+    SHOULD be implemented in the Keywords table of suitable Robot resource file
+    to enable readability and code-reuse.
+
+    + Such keywords MAY be implemented in Keywords table of the suite instead,
+      if the contributor believes no other test will use such keywords.
+      But this is NOT RECOMMENDED in general, as keywords in Resources
+      are easier to maintain.
+
+  + All test case names (and suite names) SHALL conform
+    to current naming convention.
+    https://wiki.fd.io/view/CSIT/csit-test-naming
+    TODO: Migrate the convention document to .rst and re-link.
+
+  + Frequently, different suites use the same test case layout.
+    It is RECOMMENDED to use autogeneration scripts available,
+    possibly extending them if their current functionality is not sufficient.
+
++ Resource files
+
+  + SHALL be used to implement higher-level keywords that are used in test cases
+    or other higher-level (or medium-level) keywords.
+
+  + Every keyword SHALL contain Documentation where the purpose and arguments
+    of the keyword are described. Also document types, return values,
+    and any specific assumptions the particular keyword relies on.
+
+  + A keyword usage example SHALL be the part of the Documentation.
+    The example SHALL use pipe and space separated format
+    (with escaped pipes and) with a trailing pipe.
+
+    + The reason was possbile usage of Robot's libdoc tool
+      to generate tests and resources documentation. In that case
+      example keyword usage would be rendered in table.
+
+    + TODO: We should adapt it for current tool
+      used to generate the documentation.
+
+  + Keyword name SHALL describe what the keyword does,
+    specifically and in a reasonable length (“short sentence”).
+
+    + Keyword names SHALL be short enough for call sites
+      to fit within line length limit.
+
+  + If a keyword argument has a most commonly used value, it is RECOMMENDED
+    to set it as default. This makes keyword code longer,
+    but suite code shorter, and readability (and maintainability)
+    of suites SHALL always more important.
+
+  + If there is intermediate data (created by one keyword, to be used
+    by another keyword) of singleton semantics (it is clear that the test case
+    can have at most one instance of such data, even if the instance
+    is complex, for example ${nodes}), it is RECOMMENDED to store it
+    in test variables. You SHALL document test variables read or written
+    by a keyword. This makes the test template code less verbose.
+    As soon as the data instance is not unique, you SHALL pass it around
+    via arguments and return values explicitly (this makes lower level keywords
+    more reusable and less bug prone).
+
+  + It is RECOMMENDED to pass arguments explicitly via [Arguments] line.
+    Setting test variables takes more space and is less explicit.
+    Using arguments embedded in keyword name makes them less visible,
+    and it makes it harder for the line containing the resulting long name
+    to fit into the maximum character limit, so you SHOULD NOT use them.
+
+Python library files
+~~~~~~~~~~~~~~~~~~~~
+
+TODO: Add guidelines for Python scripts (both utilities called by test on nodes
+and unrelated ones such as PAL) if there are any (in addition to library ones).
+
++ General
+
+  + SHALL be used to implement low-level keywords that are called from
+    resource files (of higher-level keywords) or from test cases.
+
+  + TODO: Discuss debugability, speed, logging, complexity of logic.
+
+  + Higher-level keywords MAY be implemented in python library file too.
+    it is RECOMMENDED especially in the case that their implementation
+    in resource file would be too difficult or impossible,
+    e.g. complex data structures or functional programming.
+
+  + Every keyword, Python module, class, method, enum SHALL contain
+    docstring with the short description and used input parameters
+    and possible return value(s) or raised exceptions.
+
+    + The docstrings SHOULD conform to
+      `PEP 257 <https://www.python.org/dev/peps/pep-0257/>`_
+      and other quality standards.
+
+    + CSIT contributions SHALL use a specific formatting for documenting
+      arguments, return values and similar.
+
+      + FIXME: Find a link which documents sthis style.
+        it is based on Sphinx, but very different from
+        `Napoleon style
+        <https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_numpy.html>`_.
+
+  + Keyword usage examples MAY be grouped and used
+    in the class/module documentation string, to provide better overview
+    of the usage and relationships between keywords.
+
+  + Keyword name SHALL describe what the keyword does,
+    specifically and in a reasonable length (“short sentence”).
+    See https://wiki.fd.io/view/CSIT/csit-test-naming
+
+  + Python implementation of a keyword is a function,
+    so its name in the python library should be lowercase_with_underscores.
+    Robot call sites should usename with first letter capitalized, and spaces.
+
+    + FIXME: create Robot keyword naming item in proper place.
+
++ Coding
+
+  + It is RECOMMENDED to use some standard development tool
+    (e.g. PyCharm Community Edition) and follow
+    `PEP-8 <https://www.python.org/dev/peps/pep-0008/>`_ recommendations.
+
+  + All python code (not only Robot libraries) SHALL adhere to PEP-8 standard.
+    This is reported by CSIT Jenkins verify job.
+
+  + Indentation: You SHALL NOT use tab for indents!
+    Indent is defined as four spaces.
+
+  + Line length: SHALL be limited to 80 characters.
+
+  + CSIT Python code assumes PYTHONPATH is set
+    to the root of cloned CSIT git repository, creating a tree of sub-packages.
+    You SHALL use that tree for importing, for example::
+
+       from resources.libraries.python.ssh import exec_cmd_no_error
+
+  + Imports SHALL be grouped in the following order:
+
+      #. standard library imports,
+      #. related third party imports,
+      #. local application/library specific imports.
+
+    You SHALL put a blank line between each group of imports.
+
+  + You SHALL use two blank lines between top-level definitions,
+    one blank line between method definitions.
+
+  + You SHALL NOT execute any active code on library import.
+
+  + You SHALL NOT use global variables inside library files.
+
+    + You MAY define constants inside library files.
+
+  + It is NOT RECOMMENDED to use hard-coded constants (e.g. numbers,
+    paths without any description). It is RECOMMENDED to use
+    configuration file(s), like /csit/resources/libraries/python/constants.py,
+    with appropriate comments.
+
+  + The code SHALL log at the lowest possible level of implementation,
+    for debugging purposes. You SHALL use same style for similar events.
+    You SHALL keep logging as verbose as necessary.
+
+  + You SHALL use the most appropriate exception not general one (Exception)
+    if possible. You SHOULD create your own exception
+    if necessary and implement there logging, level debug.
+
+    + You MAY use RuntimeException for generally unexpected failures.
+
+    + It is RECOMMENDED to use RuntimeError also for
+      infrastructure failures, e.g. losing SSH connection to SUT.
+
+      + You MAY use EnvironmentError and its cublasses instead,
+        if the distinction is informative for callers.
+
+    + It is RECOMMENDED to use AssertionError when SUT is at fault.
+
+  + For each class (e.g. exception) it is RECOMMENDED to implement __repr__()
+    which SHALL return a string usable as a constructor call
+    (including repr()ed arguments).
+    When logging, you SHOULD log the repr form, unless the internal structure
+    of the object in question would likely result in too long output.
+    This is helpful for debugging.
+
+  + For composing and formatting strings, you SHOULD use .format()
+    with named arguments.
+    Example: "repr() of name: {name!r}".format(name=name)
+
+Bash scripts and libraries
+~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+TODO: Link here when document for this is ready.