Jump to: navigation, search

Integration/Test/Test Code Guidelines

Contents

Preamble

This documents aims to aggregate code practices for Integration/Test project.

Its git repository can be viewed on primary GitWeb or on GitHub mirror.

Although integration/test contains also Python utilities and libraries, most of this page focuses on Robot Framework source files.

The motto of Robot Framework: "Well written documentation can be executed directly."

Frequently, test code contributors go inactive while their tests are still relevant and in a need of maintenance. Therefore committers usually prefer readability and maintainability; "it works" is not a reason enough for merging a sub-standard patch.

Usually, +2 from a committer means Integration/Distribution as a community is willing to maintain the contribution starting from its current state. If the committer feels the first maintenance step would be "rewrite portions of the contribution to conform to these guidelines", it is correct to vote -1 instead.

There may be times, where timely merge is more important than code quality. For example when release is near, and a potential blocking Bug needs to be tracked. In those cases, at least add TODOs and FIXMEs so that the technical debt can be repaid when time is less pressing.

Here are some guidelines for writing reliable, maintainable, reusable and readable Robot test code:

Technical guidelines

Test environment

This document is for contributors and committers of Integration/Test project, with ODL CSIT as its main execution environment. The preparatory part is generally handled by scripts in Releng/Builder project. The specific division of work is described elsewhere (FIXME: Is it? TODO: Where?).

This section comments on decisions which affect design goals of Robot tests.

From the Robot perspective, everything before "pybot" command starts is environment preparation, everything after "pybot" returns is (log gathering and) cleanup, everything in between is testing. In practice, the division is not so clear. Preparation can test readiness, tests can alter installation, tests can manipulate logs, scripts after "pybot" can run diagnosis commands. Robot suites should clearly document any "non-test" actions they are performing and why.

The execution environment can be conceptually divided to System Under Test (SUT) and Test Environment (TE). In order for test results to be informative, it is important for a conceptual Tested Interface (TI) between SUT and TE to be well understood. Test plans and scenario documentation should make it clear which data are going through which part of TI.

Robot test frequently evolve from manual tests; and if a bug is reported, developers usually use simplified manual steps to verify their patches. For those reasons, it is better if there is an easy way to figure out manual tests equivalent to the Robot code.

This is mainly important for actions, that is data TE decided to send in order to affect SUT behavior. Responses from SUT do not have to be defined so strictly, as the manual tester usually has no problem parsing them. This allows TI to be shifted somewhat. For example when testing Restconf communication, manual tester would use "curl", but the Robot test can use TemplatedRequests (currently recommended wrapper around RequestsLibrary). The TI shift is in that HTTP client library is now part of SUT with respect to Robot report, while it remains part of TE with respect to test plan.

Including test-related libraries to SUT is not bad, as long as it is clearly documented. Sometimes there is no client application for the server under test to interact with, and it is common to develop test suite and a test tool at the same time. But in that case, the developer trying to perform manual debugging should have access to the same tools, in order to ensure thet the server under test really received the correct data and that its responses were parsed correctly.

To avoid situations where test libraries are tightly integrated with Robot suite (so developer needs to run pybot, specifying all argument the suite needs), honor TI by separating Robot (TE) from test tools (SUT).

Source files

The recommended extension for Robot source files (both test cases and Resources) is .robot while .txt is for other purposes, such as testplans and readmes.

Source files are usually grouped in directories by their type, so there is csit/suites/, csit/libraries/, csit/variables/ and so on. Keep the placement in mind when referring to those files.

Concerning capitalization, Resources traditionally use CamelCase (as a call from outside looks better that way), so for suite files names it is recommended to use snake_case instead.

Suite names

Suite renaming is discouraged in general. Robot plugin for Jenkins uses file name to detect sub-suites, so history for sub-suite would be effectively lost. On the other hand, if a new suite is added to a directory where there already is an older suite with too generic filename, sub-suite history is a small price to pay, and making sub-suites distinguished better is worth it.

DON'T: Numeric prefix

Early suites had names starting with numbers, such as 010_restconf.robot but this is also slightly discouraged as people might want to renumber things upon removing or adding suites. Put suite names to testplan file to ensure intended ordering of execution.

Encoding

All robot source files should be encoded in ASCII. Non-ASCII characters are allowed but they must be encoded in UTF8 (the default Robot source file encoding). Different encodings of the source files are not supported. This includes also documentation and comments.

Trailing and separation spaces

NOTE: You can use Robot tidy tool to fix your robot file.

Some text editors compatible with robot like RIDE (recommended IDE) automatically adjusts trailing and separation spaces for your Robot test file. However if you are using a normal text editor we recommend to watch the following rules:

  • Do not use spaces-and-pipes style of separation.
  • Avoid trailing spaces at the end of any text line.
  • Use a minimum of 18 space indent in the Settings and Variables sections to separate key and value.
  • Use 4 spaces between keywords and arguments in the Test Cases and Keywords sections.

Example:

*** Settings ***
Documentation     Test suite to verify Restconf is OK
Suite Setup       RequestsLibrary.Create_Session    session    http://${ODL_SYSTEM_IP}:${RESTCONFPORT}    auth=${AUTH}    headers=${HEADERS_XML}
Suite Teardown    RequestsLibrary.Delete_All_Sessions
Library           RequestsLibrary
Variables         ${CURDIR}/../../../variables/Variables.py

*** Variables ***
${RESTCONF_MODULES_URI}    /restconf/modules    # FIXME: Variables has similar variable already.

*** Test Cases ***
Check_Controller_Modules
    [Documentation]    Get restconf modules, see ietf-restconf and http status 200.
    ${resp} =    RequestsLibrary.Get_Request    session    ${RESTCONF_MODULES_URI}
    BuiltIn.Log    ${resp.content}
    BuiltIn.Should_Be_Equal    ${200}    ${resp.status_code}    # status_code is always integer.
    BuiltIn.Should_Contain    ${resp.content}    ietf-restconf

Line length

There is no hard limit on line length. Having shorter lines (limit of 80 or 120 characters) is better for readability, but do not compromise code logic to get there.

Note that tidy tool may change line wrapping (does not apply to [Documentation]). Keep the tidy wrapping.

Automated formatting

There is a script in integration/test: tools/robot_check/tidy.sh which you can use to fix your formatting errors. Cd to its directory and run the script with argument "tidy" (without quotes). It would re-format all .robot files in your working tree, fixing all robot formatting issues.

Documentation and comments

Documentation, both as [Documentation] and as comments, is important for test code readability.

[Documentation]

Suites, Resources, Test cases and Keywords support this tag.

FIXME: Choose one capitalization for Keyword, Test Case and so on; edit this wiki to use consistent capitalization.

There is a simple rule: If there could be a [Documentation], it should be there. Even in "internal" keywords, even in short keywords where name says it all.

Content

Documentation should describe what this code does from the caller point of view. For keywords that means what the arguments are, what is returned and what caller-visible side-effects are. Implementation details (unless needed to explain side-effects) are not of interest.

The documentation is mostly read by callers deciding which code to rely on. The second most probable reader is a person trying to debug an unexpected failure.

Aside of documentation being too brief, documentation is typically bad when the text explains why this keyword was added, without describing what the (now abstracted) block of code actually does from the caller point of view.

Formatting

Documentation may get exported by automated tools. Therefore, most rules which apply to Python docstrings (See PEP 257) also apply to Robot documentation.

Of course specifics such as triple-double quotes do not apply, and tidy will take care of indentation. But rules such as (paraphrased) "first line separated by empty line" and "as a command, not as a description" have the same validity in Robot as in Python.

License

Robot files are not officially released, but contributors may still prefer to include license information.

The recommended place is in the suite/resource Documentation. The usual structure of file Documentation:

  1. First line as a summary.
  2. Empty line (with three dots to signal Documentation continues).
  3. License information block.
  4. Two empty lines.
  5. Paragraphs of detailed documentation...
  6. ... separated (but not ending with) single empty lines.

Comments

Within Keyword/TestCase implementation, the logic should be clear just from names of keywords and variables used. But sometimes comments are needed, for example to explain why a more natural implementation fails due to some non-obvious detail.

Use Python-style comments, which means '#' character followed by space. Then either a whole sentence (properly capitalized and punctuated). Only use short phrases (without capitalization or punctuation) if horizontal space is precious.

If on a separate line, indent with previous line. If at the end of line, separate by 4 spaces. Note that tidy tends to change comment placement in some cases.

AVOID: Comments as abstraction

If you find yourself adding comments to separate blocks of code which correspond to higher abstraction step, consider defining Keyword for each such step and calling that.

In some cases, the need to copy local variables would be too taxing, but in most cases it would increase readability of the code.

Commented-out code

Do not add commented-out code into suites. It would reduce readability, it would not be maintained and tidy tool could indent it wrongly.

Do not comment-out previous code, delete it. Git history remembers the code, together with context of its creation, edits and the final removal.

Libraries and Resources

Both Libraries and Resources define Robot Keywords. Libraries are written in Python (integration/test does not support libraries written in other languages such as Java, as "pybot" is the only Robot interpreter used in CSIT).

FIXME: Add link to an explanation of what CSIT (or CI) is.

System Libraries

There are many public libraries you can use, so far we have installed the following in CI for you to use:

User Libraries

Besides public libraries we can also use OpenDaylight custom libraries. These are stored in "csit/libraries/" of integration/testing repository. Example of custom libraries are "AuthStandalone.py" or "norm_json.py".

NOTE': Public libraries are normally better maintained and documented than custom libraries, for this reason we recommend the first whenever possible.

Using custom Keywords and Resources

TODO: Describe Keywords table in separate section?

It is possible to define robot keywords using other robot keywords so that we save some code. This is normally done in the Keywords section of a robot file:

*** Keywords ***
Check_Nodes_Stats
    [Arguments]    ${node}
    [Documentation]    A GET on the /node/${node} API is made and specific flow stat
    ...    strings are checked for existence.
    ${resp} =    RequestsLibrary.Get_Request    session    ${SOME_RESTCONF_API}/node/${node}
    BuiltIn.Should_Be_Equal    ${200}    ${resp.status_code}
    BuiltIn.Should_Contain    ${resp.content}    flow-capable-node-connector-statistics
    BuiltIn.Should_Contain    ${resp.content}    flow-table-statistics

A resource file is a robot file normally containing keywords that can be imported and used in a test file. We have a collection of resource files in test/csit/libraries, some examples:

  • KarafKeywords.robot: Karaf keywords like check feature is installed or run karaf command.
  • FlowLib.robot: Keywords to create/modify flow bodies.
  • Utils.robot: Variety of general keywords, it is planned to split this to smaller resources.

NOTE: Keywords written in robot are easier to debug than keywords written in Python, for this reason we recommend the first whenever possible. Keywords defined in Python ale recommended only when speed of execution is critical.

Variables can also be defined in Resources. FIXME: Find appropriate section for the previous sentence.

Importing

Import is done by adding to Settings table. System libraries do not contain path nor file extension. User libraries and Resources contain both.

Even though pybot (the command that runs Robot tests) automatically considers import paths to be relative to the .robot file doing the import, it is recommended to explicitly start the path with ${CURDIR}. This helps to avoid mistakes with other paths (for OperatingSystem or SSHLibrary) which consider paths relative to OS current working directory.

Libraries and Resources are declared at the top of the robot file in the Settings section:

*** Settings ***
Library           RequestsLibrary
Library           ${CURDIR}/../../../libraries/norm_json.py
Resource          ${CURDIR}/../../../libraries/PrefixCounting.robot

Recommended order of importing (by type) is shown. Libraries available in the system (or active virtualenv) first, then local Python libraries, then Variable files, then Robot resources. Within the same type, order alphabetically based on filename (including path), case insensitive).

Resource Naming

When you create new keywords to be used only by one suite, keep them in the suite file. When you start to see multiple suites with similar keywords defined, it is good to move the keywords into a separate Resource. It is also acceptable to place new keywords intended to be used from multiple suites into a separate Resource even when there is just one suite using them in the change that introduces them.

Having keywords scattered around many small Resource files makes suite Settings table long, but having overly broad files such as Utils.robot can be even worse for people trying to find keyword definitions. Middle ground is possible the best; but when in doubt, choose smaller Resource files.

The resource name should be short and descriptive. Note that at import, everyone sees the scit/libraries/ part, so Keywords, Library, Utils and similar words do not actually provide additional description.

Traditionally CamelCase is used for file names, but possibly including underscores (as in keyword names below) would be better.

Calling Imported Keywords

Even though Robot puts both locally defined and imported keywords into a common namespace, it is still recommended to prepend Library/Resource name (and dot) when calling imported keywords (including keywords from BuiltIn). This is to make it easier for reader to know where to look for the keyword definition.

    MyResource.My_Keyword    # FIXME: generic; also, give whole Keyword or test Case.

That way, if a keyword does not have prefix, reader knows it is defined in the same file.

Even though Robot imports transitively, it is recommended to import a Library/Resource in every file there is an explicit mention of it. That makes reader aware which documentation to study before diving into implementation details. Even more important: It makes dependency clear for people refactoring the said resource. And it also works around the fact that Robot does not support OuterResource.InnerResource.Keyword (FIXME: generic) as a keyword reference.

Keyword Naming

Content

There is an overlap between keyword name and keyword documentation.

Keyword name should describe what the keyword specifically does from the caller point of view, in a reasonable length. Usually, the right length is "short sentence". Try to make the "short sentence" to be about 15-40 characters.

As there is less space, details about arguments and return value might be omitted. As with documentation, imperative mood should be used.

Capitalization

Robot ignores capitalization, single spaces or underscores. This allows several different ways to write the name for the same keyword. The important style rule is to stick to just one way within one file, so that readers have easier time distinguishing what is the same keyword and what not.

For new files or big rewrites, the recommended capitalization is First_Letters (even if the word in question is an initialism). Minor style choice is to use underscores instead of spaces. It avoids mistake of accidental double space, it allows to include keyword name in English text without need of quotes, and it is slightly more appealing for coders (most programming languages disallow spaces in identifiers).

Good example: Dump_Tcp.

Good enough example: Dump Tcp.

Bad Examples: DumpTcp, dump_tcp, dump tcp, Dump_TCP, Dump TCP.

Naming scheme

For example, Verb_First_Into_Second_Argument_Return_Status is formally good a keyword name. FIXME: Such generic name examples may be misleading, as actual names have to be as specific as possible. Remove generic examples from this wikipage and use specific examples from actual suites.

An example scheme defining more precise meaning of Verbs (words in {braces} to be replaced with specifics of that type):

  • Get_{value}: Interact with system under test (without major side effects) and return a value of interest.
  • Should_{satisfy_condition}: Perform a simple test on the value given as argument. Simple means the test does exactly what the condition implies, documentation is not much longer than the keyword name.
  • Check_{condition}: Get a value then pass it to a Should keyword.
  • Validate_{condition}: Perform a convoluted test on the value given as argument.
  • Verify_{condition}: Either Get and Validate, or a more convoluted test involving multiple Gets.
  • Wait_For_{condition}: A loop similar to BuiltIn.Wait_Until_Keyword_Succeeds with Check or Verify inside.

Keyword name conflicts

If a suite (or parent Resource) does not prepend Resource name (and dot), and two Resources in import tree define Keyword of the same name, Robot throws an error. There are suites which do not prepend, and they are being refactored occasionally. Therefore Resource maintainers cannot assume robust calling, and they should choose Keyword names unique enough, to avoid accidental conflicts.

This is especially important for typical Keyword names such as Setup or Teardown.

TODO: Add a recommendation for Setup and Teardown naming. For example "MyResource.Setup_MyResource".

Private Keywords

Robot Framework does not have any visibility limitations with respect to keywords. If user imports a Resource, every keyword there (and every keyword imported transitively) is available. See this chapter on what to do if name clash happens.

Well-named keywords intended for user should never lead to name clash, but sometimes internal keywords used just for code structure may be accident prone.

The recommended way is to prepend Resource name and two underscores to signal a keyword is private to this resource file. For example ExampleLib__Verb_Core. (FIXME: generic)

For Python Libraries, see how to restrict visibility.

Keyword ordering

There are several viable strategies when it comes to order in which Keywords are listed in Resource (or suite). The main ones are: higher level first, lower level first, or alphabetically.

Alphabetical ordering is good for large Resources, so that reader can locate Keyword more quickly. But large Resources are discouraged, as searching for Keywords is still slow.

Lower level first is good for readers which want to read whole Resource. This way they can understand exactly what the code does without need to skip ahead. This is good for reviewing, but not necessarily for users.

Higher level first is good for users (reading from the start) when searching quickly for a suitable Keyword. Higher level Keywords usually include additional logic which makes their usage easier, but the logic may enforce conditions not suited for every test case. Lower level Keywords usually have less conditions and more customization, but users need to provide more complicated arguments. This ordering usually results in user finding the most friendly Keyword of applicable Keywords, which is why this ordering is recommended for resources.

Depending on Resource, there may be other criteria for good ordering; specify them in Documentation.

For suites, lower level first may be a better ordering, but this guide does not give specific recommendation for suites. TODO: Figure one out.

Returning from Keywords

If the keyword does not return any value, no explicit statement is needed. Return will execute at the end of keyword code.

If the cell with value to return has some processing in it, if multiple values are being returned, or if the calling code is expected to call the keyword in a complex line; it is better to use BuiltIn.Return_From_Keyword. The reason is that this logs arguments (without expanding variable names) in log.html automatically, while [Return] does not log anything.

If a single value from variable is returned (which is either ignored by caller, or assigned to a single variable in caller), it is more readable to use [Return] setting with a local variable just assigned to on the previous line.

Setting suite variables in Keywords

An alternative to returning values is putting them to suite variables (including Resource-private variables). This saves one line of code, but makes the parent suite (or Resource) way less readable, as it is not obvious how a particular variable was set (without looking into child Resource). Overall, setting suite variables is usually strongly discouraged.

The exception are Keywords whose primary use is to set several suite variables (used by several parent suites). In this case, saving lines is more important. This assumes that parent suites are documented well, so that reader knows what is going on.

Singleton Resources

If you have a set of keywords that pass values in suite variables to each other, consider placing them into a Singleton Resource. A Singleton Resource is a Resource that contains a set of keywords which access and maintain an internal state stored in Resource Private variables. This Singleton Resource approach is especially useful when these keywords need to maintain the state across test cases. For an example of a Singleton Resource see RestPerfClient.robot.

Code reuse

If you have logic common for several test cases (or keywords), extract it into a single keyword. If you have several similar keywords, consider making it one keyword with additional parameter. If two Resources seem to do very similar things, consider merging it to one Resource with parametrized keywords.

The point of code reuse is to make maintenance easier. When a change in one suite is made, it is very probable that other suites that do similar action may want the same change applied. If the action is in one Resource, other suites get it automatically. Otherwise contributor would have to search for every similar suite, or there will be a risk of similar suites using varied quality of actions.

Moving Keywords to separate Resources is decreasing readability, but maintenance is usually more important. On the other hand, call chain should not be long (it is harder to understand and it pollutes log.html) and search&replace can be cheap; so even though code re-use is encouraged, do not overdo it. TODO: Is there a criterion to tell where the boundary between "reusing too little" and "reusing too much" is?

Using Variables

There are different recommendations based on variable classification.

Variable scoping

Robot supports several different scopes for variable visibility. Do not use global scope. Do not use suite scope if possible (see below). Prefer local scope.

TODO: Update this wiki when a good example of test scope appears.

Suite variables

Suite variables are normally declared in the Variables section of a Robot file:

*** Variables ***
${BGP_VARIABLES_FOLDER}    ${CURDIR}/../../../variables/bgpuser/
${KARAF_BGPCEP_LOG_LEVEL}    ${KARAF_LOG_LEVEL}
${KARAF_LOG_LEVEL}    INFO
${last_count}     -1    # note 5 spaces to indent at 18

Besides your locally defined variables, you can import commonly used variables from test/csit/variables/Variables.py file:

*** Settings ***
Variables          ${CURDIR}/../../../variables/Variables.py

Or you can also use the variables defined in a resource file if you import the file in your test; but this is not recommended as there is no easy way to signal that the variable is defined in external file.

Overridden variables

Variables can be passed at test launch time using the robot command (pybot) option '-v variable:value'. Any variable passed at execution time has preference over any local definition.

These variables have global scope, but it is recommended to treat them as suite variables and immutable (see below). Never change those values during test execution.

CSIT passes some values automatically, and other values to pass may be configured in CSIT job definition. FIXME: Give a cross-link to CSIT job definitions.

It is slightly recommended to put such overridden variables in suite's Variables table, to remind reader the variable value will be accessed, and to give a reasonable default value. Common variables, such as ODL_SYSTEM_IP should be defined in Variables file (and not in suite's Variables table) though.

Variable Chaining

As seen in ${KARAF_BGPCEP_LOG_LEVEL} example, Robot is able to figure out order of evaluation, so suites can "chain" variable definitions to set default values constructed from other (possibly overridden) values. This is useful when running several suites, to allow both suite-dependent (${KARAF_BGPCEP_LOG_LEVEL}) and suite-independent (${KARAF_LOG_LEVEL}) value overriding.

TODO: There is a plan to convert variables.py into Variables.robot as the .py file does not handle setting default value to a variable overridden in pybot. Update this when Variables.py is officially deprecated.

Mutable suite variables

As direct data passing between test cases is not possible, mutable suite variables are sometimes required. Certainly if an object (SSH session, generated ID, or something similar) is created in Setup and has to be accessed in Test Case. Sometimes such objects are created in test cases themselves.

From this list, mutable suite variables in Robot are bad because of Non-locality and Implicit coupling. On the other hand, Concurrency issues and Memory allocation issues are practically never relevant for Robot, and other whys apply only in principle, as Robot generally does not offer enough strictness.

Within single Test Case, it is recommended to pass state between keywords in explicit return values. Mutable suite variables are tolerated only if explicit passing hurts readability too much.

Set_Suite_Variable

BuiltIn.Set_Suite_Variable supports two formats, there is a note on difference. It is slightly recommended to always use the escaped format, so that reader does not need to analyze whether the previous value could be a variable name.

Variable naming

Name of a variable should describe what kind of value is stored there. Avoid too-short or ambiguous names. There is no need for memory optimization, so do not reuse variables for multiple purposes.

If scope is larger than local, think about possible name clashes. Doubly so when developing a Resource. Note that older Robot versions had a bug where local variables were visible from called keywords; the bug was fixed, so pass all the relevant data as arguments.

Capitalization to be used is snake_case unless the variable is of the one of the special classes described below. In this capitalization abbreviations which are normally written in all caps such as "CRUD" (abbreviation for Create, Read, Update, Delete) are written lowercase as in "directory_with_crud_templates".

User configurable variables

If a value of a suite variable is not to change during test execution and it is meant to be possible to override it in pybot using -v, its name should be in ALL_CAPS and the name shall be an at least two word phrase so it includes at least one underscore. All these user configurable variables are considered immutable and thus their value shall never be changed in the suite. The "at least one underscore" is meant to distinguish these variables from Robot hard-coded variables such as ${EMPTY}, ${NONE} or ${CURDIR}, which are ALLCAPS (note the missing underscore), are not considered user-configurable, and shall always have their Robot documented value in them.

User configurable variables shall have their default values set in the Variables table. However if you need to define a default value for an user configurable variable, which is a result of an arithmetic expression involving other immutable variables, it is not possible to do so in the Variables table due to Robot Framework limitations. In that case define the default value in the Suite Setup keyword using Utils.Set_User_Configurable_Variable_Default as in the example below (note that the name of the user configurable variable is NOT enclosed in the ${}):

${value}=    BuiltIn.Evaluate    ${REQUEST_COUNT}/50+10
Utils.Set_User_Configurable_Variable_Default    DIRECT_MDSAL_TIMEOUT    ${value}

The Utils.Set_User_Configurable_Variable_Default keyword makes sure that if the variable value is set by the user in pybot using -v, the default value is ignored and the user supplied value in the variable is left intact. When using this keyword DO NOT set the variable to anything in the Variables table, otherwise the value set in the Variables table will be taken as the default and the value set using this keyword will be silently ignored.

On the other hand, all variables created during test execution are obviously not user configurable, even if the firstly assigned value never changes. This means local variables and mutable suite variables use snake_case. Also immutable variables which are just a shorthand for too-long or frequently repeated value and which are not meant to be overridden use snake_case, even when set in the Variables table.

TODO: Immutable variables which are not meant to be configured by user (unless he has intimate knowledge of the test case and the resources it is using) shall have their own distinct casing style (different from ALL_CAPS and snake_case). There is some confusion about which variable is which if they both use snake_case. An example of such variable is "directory_with_template_folders" which is immutable but if the user overrides its default value, the tests won't work at all (if the new value does not point to the directory with the correct data) or they won't test what they claim they do (if the new value does points to a directory with data that instruct ODL to do something different than the test case using them wants to do).

Resource-private variables

Sometime it makes sense to keep some value tied to a Resource file. Commonly this is done for expensive and reusable objects, such as SSH or HTTP sessions. Naming similar to private keywords is recommended:

*** Variables ***
${KarafKeywords__karaf_connection_index}    -1
${KarafKeywords__odl_operational_restconf_uri}    https://${ODL_SYSTEM_IP}:${ODL_RESTCONF_PORT}/operational

Note that suites should always declare their suite variables in Variables table (or at least always create them before reading), so name repetition is not needed in suites, only in Resources.

Examples

Good examples: ${tcp_dump}, ${DEFAULT_TCP_DUMP}.

Bad examples: ${tcpdump}, ${tcp dump}, ${tcpDump}, ${tcp/dump}, ${TCP_dump}, ${Tcp_Dump}, ${TCP_DUMP}, ${default_tcp_dump}, ${DEFAULT TCP DUMP}.

Variable assignment

Local variables are assigned to similarly as in programming languages. Robot Framework allows 3 different syntaxes for variable to be assigned to: just variable, variable and equals sign (as in bash) or variable space equals (as in Python). It is strongly recommended to use equals sign, as it improves readability. Bash or Python is more of a stylistic choice. Python style may be more familiar (thus readable) but there a slight risk of accidental double space.

Note that assignment to a local variable requires the second cell to be a keyword. If you have a raw value to assign, use Set_Variable.

Use Set_Suite_Variable for setting suite variables. Note that order (left hand side is after keyword) is different from Set_Variable. Also, equals sign cannot be used in Set_Suite_Variable.

Assignment_Example
    ${local_variable} =    BuiltIn.Set_Variable    string_value    # FIXME: generic
    BuiltIn.Set_Suite_Variable    ${suite_variable}    0    # also a string value

String values

When you are free to choose string data for your tests, it is another opportunity for increasing readability of your test code.

As the string value may also be entered as a default value for a named argument, it is recommended for the value to be short and without spaces. For example, you may use "bad_password" or "changeme" for an incorrect password value, and "good_password" or "topsecret" for a correct password value.

String parts

When putting a substring into a variable, do not include surrounding delimiters. In the usage, readability is more important than saving a character or two.

For example, /${REST_OPER}/${TOPO}/my-topology is more readable than ${REST_OPER}${TOPO}my-topology.

Keyword arguments

Python distinguishes between arguments and parameters but Robot usually talks about arguments, rarely distinguishing argument names from argument values.

Argument types

Similarly to Python, keyword definition can specify its arguments in four different ways and call to a keyword can specify argument values in named or un-named fashion.

Support for "*args" and "**kwargs" is limited, using list variables and dict variables (@{args} and &{kwargs}), but that can mess up rules of how additional positional arguments are applied. Also, even empty @{args} is detected as a positional argument present, therefore it is strongly recommended to list all arguments explicitly if possible.

Usually, arguments with defaults and named arguments result in a more readable code. Positional arguments are preferred only if horizontal space is precious, or if there is a logic that forwards arguments without knowing names for them (somewhat common in more advanced Resources, as Robot lacks direct support for functional programming).

Argument naming

From keyword code block, references to arguments should be treated as immutable local variables.

TODO:This rule tends to break down the code if you need to put a nontrivial default value to the argument because Robot Framework has a shortcoming that prevents easy use of that. The following construct (or a similar one based on this) as shown on this example should be allowed:

Compose_Base_Java_Command
    [Arguments]    ${openjdk}=${EMPTY}
        <em>... docs and settings for the keyword ...</em>
    ${default}=    BuiltIn.Get_Variable_Value    ${JDKVERSION}    none
    ${openjdk}=    BuiltIn.Set_Variable_If    """${openjdk}"""==""    ${default}    ${openjdk}
        <em>... from now on ${openjdk} has the correct value and should be treated as read-only ...</em>

The Robot Framework bug here is that you can't set the default value of ${openjdk} to ${JDKVERSION} and expect the whole thing to behave correctly if you want to make it optional. Maybe a "special keyword" should be created to facilitate readability of these places in the code.

UPDATE: The example above is not actually relevant if you use Robot Framework 3.0 which has the bug fixed (if you specify a default value for a variable in the [Variables] table and then use that variable as a default value of an argument and then an user overrides the default value for the variable, Robot Framework 3.0 will correctly use the overriden value of the variable as the default value of the argument).

Names of arguments should be in snake_case and they should be descriptive, if somewhat shorter than ordinary variable names (to save space in [Arguments] line).

Default argument values

In order to avoid code duplication, it is recommended to create a few "rich" keywords with many optional arguments, instead of many "narrow" keywords with no optional arguments.

When choosing the default values for optional arguments, choose in such a way that minimum overrides in call leads to high quality operation. This usually means these considerations.

Logging verbosity

When deciding on amount of information Logged, usually more is better, as you never know what could be useful for debugging. Let callers explicitly turn off logging when they expect data is too big to look at.

The less usual situation is with logs of "trace" type. It this case the default value should disable them, only callers with intention to deeply debug will enable them. But this setup is somewhat discouraged, as the trace type logging takes code space.

There is no clear line between the two cases. In any way, the defalut values should result in reasonable amount of logging.

Test strictness

When deciding on strictness of test, stricter is better. This will help catch unexpected failures which may go undetected many lines until something only vaguely related fails. This will force callers to specify which symptoms are to be tolerated.

Computed values

Similarly to Python, expressions for default values are evaluated only once, at the time the suite/resource is loaded the first time. Therefore it is not easy to use values which depend on some variables computed in runtime. The typical example happens in cluster testing, when the list of all member indexes needs to be computed from NUM_ODL_SYSTEM.

The recommended solution is to use ${EMPTY} as a placeholder in the default value, but compute the real value when ${EMPTY} is detected. This behavior has to be clearly documented in Documentation, as sometimes ${EMPTY} is a reasonable verbatim value to use.

You may use Python short expressions, for example "${value} or the full list" stands for "if ${value} is ${EMPTY} use the full list, otherwise use ${value}".

In order to avoid discussion on argument naming, it is recommended to use different variable name to store the real value to be used. For example:

    ${computed_default} =    MyResource.Compute_The_Default_Value    @{stuff}
    ${real_argument} =    BuiltIn.Set_Variable_If    "${given_argument}" == "${EMPTY}"    ${computed_default}    ${given_argument}
    MyResource.Apply_The_Value    ${real_argument}

Code modularity

Monolithic code is hard to navigate, so it is advisable to regroup code into smaller pieces (perhaps forming a hierarchy) stored in aptly named files (and directories). If there is a part of overall state which is only accessed in one such piece (or sub-tree), it should be only defined in that piece (or sub-tree root).

Consequences for suite hierarchy is described elsewhere (FIXME: Where? TODO: Reorganize.) But there are also consequences for keywords and variables.

Keyword placement

Is the keyword only used by one suite, and unlikely to be used by other suites? Place it into the suite. Does the keyword operation sound similar to what an existing Resource does? Place it into the Resource. In general, keyword should be placed in as narrow place as possible without causing duplication.

Narrowness example: Is a keyword in SSHKeywords.robot only relevant for ssh connections to Karaf console? Move it to KarafKeywords.robot

Counter-example: Utils.robot

Utils.robot is not narrow at all. Each keyword should be moved to a more specifically named Resource. Alternatively, Documentation should describe why this Keyword is so weird that it has to remain in a generally-named Resource.

Variable placement

Is the variable only relevant to one suite? Move is into Variable table of that suite. Is the variable only used by a particular Resource? Define it in Variable table of that Resource. In general, variable should be placed in as narrow place as possible without causing duplication.

Duplication example: Is the variable only relevant for sending Restconf requests using RequestsLibrary? There is a chance TemplatedRequests already has it defined, you can call its keywords instead of invoking RequestsLibrary directly in your Resource.

Counter-example: Variables.robot

Variables.robot is not narrow at all, and it ends up included by almost every suite. Each variable should be moved from Variables.robot into a more appropriate Resource (or even suite). Alternatively, line comment should explain why this variable is so general it has to stay in Variables.robot

Consequence: Each variable in Variables.robot should have a comment, either an explanation or TODO to move it.

Timing

Good tests should be fast. Here are recommendations to keep them that way.

Timeouts

Some test steps may take indefinite time to finish, in worst case never. This is of course not acceptable, so good tests employ various timeouts to ensure test execution time remains reasonable.

A rule of thumb for the timeout duration is "twice of what it usually takes in bad case, plus whatever time a nervous manual tester would be willing to wait for".

Jenkins timeouts

Releng/Builder allows setting maximum time for test job execution. This leads to a red dot, and no results or logs are exported. That is bad, so use this as a foolproof limit, but also employ other ways to make your tests finish more gracefully.

Robot hard timeouts

Hard timeouts are able to interrupt any test step currently running.

Multiple Robot libraries have keywords which already support a timeout argument.

Robot allows timeout settings at various levels. Use them whenever there is a chance a particular step (at any abstraction level) can take too long. But if your test scenario expects a timeout to happen, and you want to react to that, it can be tricky to get right in the current (3.0.2) Robot version. FIXME: Vratko Polak could swear he had prepared a Resource (it used Test Teardown somehow) for that once; remind him to re-create and contribute that.

Soft timeouts

Soft timeouts cannot interrupt test step execution, but they can be inserted in cycles.

The typical example is BuiltIn.Wait_Until_Keyword_Succeeds (also known as WUKS). It is recommended to use this keyword whenever your test is repeatedly polling for a state change (or lack thereof). Typical example is "readiness" test case, which makes sure a particular component has finished booting up.

If the expected wait time is in seconds, one second delay between polls is fine.

Note that the duration of polls does not count into delay between polls. Therefore always use time (60s) instead of repetitions (60x) as the first argument.

Simple negative tests should use WaitForFailure.robot resource.

NetconfKeywords.robot implements a particular way to restrict the duration of a cycle without restricting the time for one iteration. TODO: Abstract that for others to re-use.

Fail fast

If there are test failures which suggest system is not prepared correctly (or got broken during testing), it can be expected most other test cases will fail as well, so it is good idea to declare some test cases failed without executing them. See Fail-fast design.

On the other hand, executing other tests shows which tests still pass, thus collecting more results and perhaps allowing an insight on which component has failed. Current timing is such that basic functional tests run fast compared to environment preparation, so it is good to try every test case. But some convoluted tests or scale/performance (not to mention longevity) tests take long enough to justify failing fast.

Robot already has a property that all test cases are marked as failed (and not executed) if Suite Setup fails. If your suite does not wish to do all relevant checks in Setup, there is FailFast.robot resource (already included by SetupUtils.robot).

Some long test cases can have more complex condition to decide when to give up to save time. See WaitUtils.robot for (equally complex) keywords to re-use.

Python code in Robot

This section is not about Libraries implemented in Python, this is about Pieces of Robot file interpreted as Python code.

Evaluate

Robot framework allows to embed small Python expressions using BuiltIn.Evaluate. It is not recommended to use that often, as not every reader is fluent in Python.

Evaluate could be used only if alternative implementation in Robot would be way less readable. For example use string.Template().substitute() if multiple text replacements are needed and you already have a dict of rules to apply, instead of extracting rules and applying multiple String.Replace_String.

Do_Something_If

BuiltIn also offers multiple keywords ending on _If (and some ending on _Unless). Typical example is BuiltIn.Run_Keyword_If. The first argument of such keywords is interpreted (after substituting Robot variables and other expressions) as a Python code to evaluate.

The safe and readable way is to fill a Robot variable with "True" or "False", but that takes vertical space. Some conditions are best to keep in Python. Typical example (and the recommended style for this expression) is evaluating "${status}" != "PASS" where ${status} comes from BuiltIn.Run_Keyword_And_Ignore_Error.

Note that ${status} contains text, so we need to quote it for Python to recognize it as a string. Dropping the quotes is an easy way to convert a string representation of a number into an int.

Things get complicated when your Python expression contains text which might contain quotes. It is recommended to use triple-double quotes to keep it as a Python string, as those are usually reserved for docstrings, thus unlikely to appear in test data or tested system output. If the Python sub-expression is safe, use double quotes, falling back to single quotes if your string contains double quotes.

Example:

"""${output}""" == '{"output":{}}'

Security considerations

Each type of quoting can be "un-quoted" by maliciously constructed text values, thus enabling execution of arbitrary Python code, which of course can be very bad.

So, do not rely on Python evaluation if you do not believe encountered values are safe (enough).

Test case Failing

Each Test Case should contain some checks or asserts. Sometimes, Test Case is intended as an intermediate environment manipulation step, without an easy way to verify its success. In that case, clarify this role in Documentation.

Checks are usually self-explanatory. But sometimes the resulting fail message turns out to be not very helpful. In this case, try to add a custom failure message. For example, both BuiltIn.Should_Be_Equal and BuiltIn.Should_Contain support (as their third argument) message to print if they fail. This message is visible both in log.html and console output.

If a failure is a known Bug already reported in Bugzilla, use Utils.Report_Failure_Due_To_Bug (or an equivalent) as a test case Teardown, so that people looking at test results know there is no need to open a new Bug.

Failure messages

BuiltIn.Run_Keyword_And_Ignore_Error allows Robot code to capture the fail message and parse it. Parsing typically uses BuiltIn.Run_Keyword_If or similar with Python expression like this:

BuiltIn.Return_From_Keyword_If    "Closed" in """${message}"""    True

See previous section on why triple-double quotes are recommended.

If a keyword wraps a failure message, it is possible to accumulate three double-quotes, creating a parsing error. To reduce such risk, it is recommended to not enclose the inner message in quotes, instead append them at the end like this:

BuiltIn.Fail    Time limit reached. Latest error: ${message}

OS specifics

Robot framework offers ways to make the test code not dependent on the operating system running it.

But integration/test currently assumes the code will be run on Linux, so you do not need to spend cycles avoiding this assumption.

Test data

Large data

If your test needs a binary data or multi-line strings, it is recommended putting them into files (as opposed to pasting them to Suite or Resource file directly. Name the files appropriately (using meaningful extensions such as .uri, .json and .xml) and use OperatingSystem.Get_File to load the data into a variable.

Templated data

If additional processing inside Robot is needed, you can use Keywords from TemplatedRequests for simple template substitutions, or you can create your own Keywords for processing your template.

Do not write processed data back to files, pass them as arguments and return values, or store them into variables of appropriate scope.

Should Be Equal argument order

Recently, BuiltIn.Should_Be_Equal reports also diff-like report when comparison between multi-line strings fail. Some people may prefer the expected value to be given as first argument to the diff, so they would use corresponding order of arguments, which happen to be opposite of what Robot Framework User Guide uses in their examples.

TODO: Put general "Do as in RFUG examples" rule in more prominent place.

TODO: Add list of acronyms, such as WUKS to a proper place.

Stay away from using a Sleep

Do not use hard coded delays or sleeps, if at all possible. Determine a concrete action you can poll on to determine if your test is ready to continue. As an example, see the keyword below. After disabling authentication on the controller it may take a few seconds for the controller to actually respond to unauthenticated requests. The "Wait Until Keyword Succeeds" keyword is used to poll every second for 10 seconds. This may not seem necessary in this instance, but the time savings can be a lot greater in other situations and as more and more automation is added, every second counts.

    Disable_Authentication_And_ReEnable_Authentication
        [Documentation]    Toggles authentication off and verifies that no login credentials are needed for REST transactions
        Disable_Authentication_On_Controller    ${ODL_SYSTEM_IP}
        BuiltIn.Wait_Until_Keyword_Succeeds    10s    1s    Make_Rest_Transaction    200
        Enable_Authentication_On_Controller    ${ODL_SYSTEM_IP}
        BuiltIn.Wait_Until_Keyword_Succeeds    10s    1s    Validate_That_Authentication_Fails_With_Wrong Token
        ${auth_token} =    Get_Auth_Token
        Make_Rest_Transaction    200    ${auth_token}

The main reason is to save precious running time. If your sleep is under 1 second (and not in a loop) it is usually OK to keep it that way. Also if the action in Wait_Until_Keyword_Succeeds would have such an overhead that it will not be run multiple times anyway, it is OK to use Sleep.

In both circumstances, use reason argument to inform readers why are the sleep is OK.

Avoid SSHLibrary.Execute_Command, use SSHKeywords.Invoke instead

FIXME:This guideline won't work until https://git.opendaylight.org/gerrit/39314 gets merged

SSHLibrary.Execute_Command is often expected to be equivalent to SSHLibrary.Write followed by SSHLibrary.Read_Until_Prompt and then "automagically" stripping everything that was not produced by the invoked command and "automagically" restoring the . However actual behavior of SSHLibrary.Execute_Command is to invoke the command in unexpected and undocumented state of the environment. More specifically, the documentation for this keyword (http://robotframework.org/SSHLibrary/latest/SSHLibrary.html#Execute%20Command) only states that "The command is always executed in a new shell. Thus possible changes to the environment (e.g. changing working directory) are not visible to the later keywords" but it does not state, how that "new shell" is actually created. The wording seems to suggest the "new shell" is a subshell of the shell which is running on the connection but tests revealed that this is not the case.

This leads to subtle bugs that may cause the suite to fail unexpectedly when run on different environment than the sandbox/releng one or when the sandbox/releng environment is upgraded. Many commands rely on the environment being configured in a certain way (as done by a login shell) and will likely misbehave if the shell running them turns out to be not configured as a login shell or its subshell. Additionally, many distributions configure various commands with user specific settings which may not be applied in the undefined and unspecified environment in the "new shell" the SSHLibrary.Execute_Command runs in.

To avoid these problems use SSHKeywords.Invoke to invoke commands on remote systems in an environment equivalent to what is used by SSHLibrary.Write.

As an example here is an incorrectly defined keyword that determines how to call the Java Virtual Machine command and returns a string ready to be appended with arguments and sent to a shell. It tries to determine where Java is installed using the value of JAVA_HOME. This fails if the Java is installed locally with JAVA_HOME set in ~/.bash_profile. A kludgy workaround for this problem along with a massive comment explaining the kludge can also be seen. This kludge works by allowing the user to set the JAVA_HOME in the VM where the Robot suite calling this keyword is running instead of the VM to which the active SSH connection is talking. The problematic line of the code below is the ${java}= SSHLibrary.Execute_Command echo \$JAVA_HOME/bin/java 2>&1 line:

Compose_Java_Invocation
    # Attempt to call plain "java" command directly. If it works, return it.
    ${out}    ${rc}=    SSHLibrary.Execute_Command    java -version 2>&1    return_rc=True
    BuiltIn.Return_From_Keyword_If    ${rc} == 0    java
    # Query the virtual machine for the JAVA_HOME environment variable and
    # use it to assemble a (hopefully) working command. If that worked out,
    # return the result.
    ${java}=    SSHLibrary.Execute_Command    echo \$JAVA_HOME/bin/java 2>&1
    ${out}    ${rc}=    SSHLibrary.Execute_Command    ${java} -version 2>&1    return_rc=True
    BuiltIn.Return_From_Keyword_If    ${rc} == 0    ${java}
    # There are bizzare test environment setups where the (correct) JAVA_HOME
    # is set in the VM where Robot is running but not in the VM where the
    # tools are supposed to run (usually because these two are really one
    # and the same system and idiosyncracies of BASH prevent easy injection
    # of the JAVA_HOME environment variable into a place where connections
    # made by SSHLibrary would pick it up). So try to use that value to
    # create a java command and check that it works.
    ${JAVA_HOME}=    OperatingSystem.Get_Environment_Variable    JAVA_HOME    ${EMPTY}
    ${java}=    BuiltIn.Set_Variable_If    """${JAVA_HOME}"""!=""    ${JAVA_HOME}/bin/java    false
    ${out}    ${rc}=    SSHLibrary.Execute_Command    ${java} -version 2>&1    return_rc=True
    BuiltIn.Return_From_Keyword_If    ${rc} == 0    ${java}
    # Nothing works, most likely java is not installed at all on the target
    # machine or it is hopelesly lost. Bail out with a helpful message
    # telling the user how to make it accessible for the script.
    BuiltIn.Fail    Unable to find Java; specify \${JDKVERSION}, put it to your PATH or set JAVA_HOME environment variable.

The correct code uses SSHKeywords.Invoke which makes sure the execution environment for the command is configured correctly. This makes any kludges (as seen above) unnecessary so the code is a lot simpler, a lot more straightforward and much easier to use (no need for tricky setups):

Compose_Java_Invocation
    # Attempt to call plain "java" command directly. If it works, return it.
    ${out}    ${rc}=    SSHKeywords.Invoke    java -version 2>&1    return_rc=True
    BuiltIn.Return_From_Keyword_If    ${rc} == 0    java
    # Query the virtual machine for the JAVA_HOME environment variable and
    # use it to assemble a (hopefully) working command. If that worked out,
    # return the result.
    ${java}=    SSHKeywords.Invoke    echo \$JAVA_HOME/bin/java 2>&1
    ${out}    ${rc}=    SSHLibrary.Invoke    ${java} -version 2>&1    return_rc=True
    BuiltIn.Return_From_Keyword_If    ${rc} == 0    ${java}
    # Nothing works, most likely java is not installed at all on the target
    # machine or it is hopelesly lost. Bail out with a helpful message
    # telling the user how to make it accessible for the script.
    BuiltIn.Fail    Unable to find Java; specify \${JDKVERSION}, put it to your PATH or set JAVA_HOME environment variable.

WARNING: The current code is full of bare SSHLibrary.Execute_Command invocations which until now did not causse any significant problems in Sandbox or Releng. However these may cause problems in the future when the environments are upgraded or they may cause problems when you try to run these suites locally. If you run into any problems while running a suite and you manage to track the problem down to a bare SSHLibrary.Execute_Command invocation, replace it with SSHKeywords.Invoke invocation and then see if the problem persists.

Make each test case standalone

FIXME: There is way too many suites where the tests must be executed in the order specified and they cannot be executed on their own. Additionally, there are some suites (their names have the "ready" word in them and the first line of their documentation contains the word "readiness" somewhere in them, e.g. csit/suites/netconf/ready/netconfready.robot) which must be the first suite executed and which contain test cases that detect bugs and apply workarounds before failing so the other test suites still have a chance to detect interesting problems. Some test suites also contain such test cases for example csit/suites/bgpcep/tcpmd5user/tcpmd5user.robot. Trying to convert these test suites to "set of standalone test cases" could easily lead to excessive time consumption as the tests would be required to do the same setup and teardown each (e.g. creating connections to the ODL and tools systems and then destroying these connections). Maybe it is time to change this to "Make each test suite standalone". See also the Discussion below.

With Robot it's easy to cherry pick a single test case to run from a full suite using tags. If that test case relies on work done in a previous test case, then this benefit cannot be realized. Each test case can use a "Test Setup" to ensure it is prepared to continue. A "Test Teardown" can be used to clean anything that might be left over and conflict with other tests that may run after it. Additionally, more basic setup and teardown can be done in the "Suite Setup" and "Suite Teardown" sections. There may be more global needs for the entire suite (e.g. configuring clustering) that only need to be done once and not before each test case.

    Add_Flows_And_Verify_Across_All_Controllers_In_Cluster
        [Documentation]    Requesting a flow creation from one controller should be seen in the operational store of all
        ...    controllers in the cluster.
        [Setup]    Start_Single_Switch_Mininet
        ${dpid} =    Get_DPID_Of_Connected_Switch    ${ODL_SYSTEM_IP}
        Push_Flow_To_DPID    ${ODL_SYSTEM_IP}    ${dpid}    ${flow}
        Verify_Flow_Exists_On_Each_Controller_In_Cluster
        [Teardown]    Clean_Mininet_And_Ensure_All_Flows_Are_Removed

The above Setup and Teardown sections would be keywords defined in the suite's *** Keywords *** section itself, or possibly from a common library. Furthermore, if most test cases will use the same test case setup and teardown, you can define it once in the *** Settings *** section using [Test Setup] and [Test Teardown].

Discussion

TODO: Create a mail thread or a wiki page for discussion on whether Robot Test Cases should be the standalone blocks as described above, or they are merely a test steps or asserts, while the standalone block is a Robot suite file (CSIT testplan then contains standalone suites). Put link here.

Scenarios and structuring

If the discussion above agrees that Robot Suite File is the unit of standalone test execution, it means leaf suite can contain at most one test scenario.

If several scenarios form a "suite" of similar tests, you can use ability of pybot to execute on directories and create structured tree of layered suites. In this case, put the top directory in testplan, but make sure alphabetical order of subdirectories and file names gives the order of execution you expect.

TODO: Compare top-directory testplan with each-file testplan to see what are differences in log.html; if none, rewrite this section to keep recommending each-file testplan.

Try to ensure the environment is clean before starting the test suite

Although it's a good idea and standard practice to clean the test environment at the conclusion of a test, it's a good idea to ensure the environment is clean and prepared at the beginning of each suite and/or test case. Robot provides a [Suite Setup] and [Test Setup] to use in this case. Unlike typical test case execution, the Setup steps will all executed even if there is trouble in one step. This also applies to [Suite Teardown] and [Test Teardown]. This is especially helpful for the case when suites are run back to back. An earlier suite may leave the system in an undesired state, but with a robust [Suite Setup], we can attempt to catch those issues and rectify if possible. As a tangible example, using the AAA test case above, we could end up leaving the controller with authentication disabled. That could be a quick and easy step to a [Suite Setup] (among many others) to validate the proper state of authentication.

Over time, a common, robust Keyword covering this type of "cleaning" can be developed.

Test cases should be short and readable

One main benefit of Robot test code is that it's very human readable. Keywords can consist of other keywords and test logic can be layered such that the high level test case reads very clearly and even a non programmer can read it and understand the basic idea of the test.

As an example, take this test case:

    Add_And_Delete_Flows_And_Verify_On_Every_Member_In_Cluster
        [Documentation]    Push a flow via northbound REST.  Verify the flow exists on the switch, as well as in 
        ...    each controllers operational and config data stores.  Next delete the flow via northbound REST and
        ...    verify it's gone from the switch and controller data stores.
        ${dpid} =    Get_DPID_Of_Connected_Switch    ${ODL_SYSTEM_1_IP}
        ${flow_id} =  Get_Last_Octet_Of_DPID    ${dpid}
        ${flow} =     Create_Generic_Flow    ${flow_id}
        Push_Flow_To_DPID    ${ODL_SYSTEM_1_IP}    ${dpid}    ${flow}
        Verify_Flow_On_Switch    ${switch_ip}    ${flow}
        Verify_Flow_In_Controller_Operational_Store    ${ODL_SYSTEM_1_IP}    ${flow}
        Verify_Flow_In_Controller_Operational_Store    ${ODL_SYSTEM_2_IP}    ${flow}
        Verify_Flow_In_Controller_Operational_Store    ${ODL_SYSTEM_3_IP}    ${flow}
        Verify_Flow_In_Controller_Config_Store    ${ODL_SYSTEM_1_IP}    ${flow}
        Verify_Flow_In_Controller_Config_Store    ${ODL_SYSTEM_2_IP}    ${flow}
        Verify_Flow_In_Controller_Config_Store    ${ODL_SYSTEM_3_IP}    ${flow}
        Delete_Flow_On_DPID    ${CONTROLLER_1}    ${dpid}    ${flow}
        Verify_Flow_Not_On_Switch    ${switch_ip}    ${flow}
        Verify_Flow_In_Not_Controller_Operational_Store    ${ODL_SYSTEM_1_IP}    ${flow}
        Verify_Flow_In_Not_Controller_Operational_Store    ${ODL_SYSTEM_2_IP}    ${flow}
        Verify_Flow_In_Not_Controller_Operational_Store    ${ODL_SYSTEM_3_IP}    ${flow}
        Verify_Flow_In_Not_Controller_Config_Store    ${ODL_SYSTEM_1_IP}    ${flow}
        Verify_Flow_In_Not_Controller_Config_Store    ${ODL_SYSTEM_2_IP}    ${flow}
        Verify_Flow_In_Not_Controller_Config_Store    ${ODL_SYSTEM_3_IP}    ${flow}

The above is rather tame, but should illustrate lots of noise that can be moved to keywords in the relevant suite itself, or maybe a common library. Most likely, this test suite would have other test cases that can make use of these types of keywords, and lots of duplication can be removed. We can end up with a much more readable and clear test case. If needed, we can drill down in to the relevant keywords to understand more details.

    Add_And_Delete_Flows_And_Verify_On_Every_Member_In_Cluster
        [Documentation]    Push a flow via northbound REST.  Verify the flow exists on the switch, as well as in 
        ...    each controllers operational and config data stores.  Next delete the flow via northbound REST and
        ...    verify it's gone from the switch and controller data stores.
        ${flow}    ${dpid} =    Get_DPID_And_Create_Flow_Data
        Push_Flow_To_DPID    ${ODL_SYSTEM_1_IP}    ${dpid}    ${flow}
        Verify_Flow_Exists_On_Switch    ${switch_ip}    ${flow}
        Verify_Flow_Is_In_Both_Data_Stores_On_All_Controllers
        Delete_Flow_On DPID    ${ODL_SYSTEM_1_IP}    ${dpid}    ${flow}
        Verify_Flow_Does_Not_Exist_On_Switch    ${switch_ip}    ${flow}
        Verify_Flow_Is_Not_In_Both_Data_Stores_On_All_Controllers

Make use, and enhance existing ODL libraries

We do auto-generate html docs based off our robot libraries which may make it easier to understand and consume.

Sharing and enhancing common libraries is an efficient way to quickly add new and better tests.

Complete Documentation

The suite level documentation should describe the overall strategy and purpose of the entire test suite. Each test case should have a documentation section describing the steps taken and expected outcome. Using Robot suites as the actual test plan itself is one method of tying the actual test code to the plan. Test cases can be stubbed out with details about the plan for each test in the documentation section, with actual test code coming at a later time. If checked in, the "test plan" in robot form can be reviewed and vetted by others by just looking in the integration/test repository.

If the test suite requires an external dependency (e.g. only works with virtual switch "X") then it should be noted in the suite level documentation.

Maybe your test suite is better in a data-driven approach

Robot Framework allows a test case to use a [Test Template] to do the work, using just the arguments given in the test case section. This can be very efficient when the test plan consists of the same basic set of steps with only a few changes in input.

Here is an example from test/csit/suites/openflowplugin/Flows_OF13/305__ttl.robot with some lines removed for brevity. The basic format of all the test cases is the same, but the inputs to the test case are different. The first test case uses a flow to set the TTL whereas the second case wants to decrement the TTL. Other arguments used in the template are things like flow ID, etc. The actual meat of the test case can be seen in the Keywords section, although this keyword could have come from a common library as well.

FIXME: Rework the source file to conform with latest style recommendations.

    *** Settings ***
    Test Template     Create And Remove Flow
    
    *** Test Cases ***    ODL flow action        action key             action value    tableID    flowID    verify OVS?    OVS specific string?
    Set_IP_TTL            [Documentation]        ${set_ip_ttl_doc}
                          [Tags]                 ttl                    set
                          set-nw-ttl-action      nw-ttl                 1               2          101       no             set_ttl
    
    Dec_TTL               [Documentation]        ${dec_ttl_doc}
                          [Tags]                 ttl                    dec
                          dec-nw-ttl             none                   none            3          305       yes            dec_ttl
    
    Copy_TTL_In           [Documentation]        ${copy_ttl_in_doc}
                          [Tags]                 ttl                    copyin
                          copy-ttl-in            none                   none            9          202       no             copy_ttl_in
    
    *** Keywords ***
    Create And Remove Flow
        [Arguments]    ${flow_action}    ${action_key}    ${action_value}    ${table_id}    ${flow_id}    ${verify_switch_flag}
        ...    ${additional_ovs_flowelements}
        @{OVS_FLOWELEMENTS}    Create List    dl_dst=${eth_dst}    table=${table_id}    dl_src=${eth_src}    nw_src=${ipv4_src}    nw_dst=${ipv4_dst}
        ...    ${additional_ovs_flowelements}
        ##The dictionaries here will be used to populate the match and action elements of the flow mod
        ${ethernet_match_dict}=    Create Dictionary    type=${eth_type}    destination=${eth_dst}    source=${eth_src}
        ${ipv4_match_dict}=    Create Dictionary    source=${ipv4_src}    destination=${ipv4_dst}
        ##flow is a python Object to build flow details, including the xml format to send to controller
        ${flow}=    Create Inventory Flow
        Set "${flow}" "table_id" With "${table_id}"
        Set "${flow}" "id" With "${flow_id}"
        Clear Flow Actions    ${flow}
        Set Flow Action    ${flow}    0    0    ${flow_action}
        Set Flow Ethernet Match    ${flow}    ${ethernet_match_dict}
        Set Flow IPv4 Match    ${flow}    ${ipv4_match_dict}
        ##If the ${flow_action} contains the string "set" we need to include a deeper action detail (e.g. set-ttl needs a element to indicate the value to set it to)
        Run Keyword If    "set" in "${flow_action}"    Add Flow XML Element    ${flow}    ${action_key}    ${action_value}    instructions/instruction/apply-actions/action/${flow_action}
        Log    Flow XML is ${flow.xml}
        Add Flow To Controller And Verify    ${flow.xml}    ${node_id}    ${flow.table_id}    ${flow.id}
        Run Keyword If    "${verify_switch_flag}" == "yes"    Verify Flow On Mininet Switch    ${OVS_FLOWELEMENTS}
        Remove Flow From Controller And Verify    ${flow.xml}    ${node_id}    ${flow.table_id}    ${flow.id}
        Run Keyword If    "${verify_switch_flag}" == "yes"    Verify Flow Does Not Exist On Mininet Switch    ${OVS_FLOWELEMENTS}

Comparison with other guides

There are other sources providing guidelines on Robot framework usage. This section compares those recommendations with Integration/Test recommendations.

Robot Framework Dos And Don'ts

http://www.slideshare.net/pekkaklarck/robot-framework-dos-and-donts

Most recommendations apply for Integration/Test; but some not, due to different priorities.

TODO: Add references to sections when telling what we recommend.

The list of what does not directly apply:

Move logic to libraries when possible

In Integration/Test, the ability to debug is more important than speed. Therefore Python libraries are discouraged, use Resource files instead.

Complex logic in test data

Some Resources need complex logic.

If a resource has only one caller in a test case, inlining the logic saves call depth. On the other hand, maintaining sufficient abstraction in test cases has its benefits. No clear recommendation on this one, it may depend on the length of the logic to be inlined and overall abstraction level in the suite.

Variable assignment on test case level

Committers do not hear about the need to hide variables. On the other hand, there were several hard-to-debug issues which would be easier-to-debug if state was passed explicitly (not using test case variables in lower keywords).

In Integration/Test, we definitely prefer test case level variable assignment from state hiding.

Move suite setup/teardown to directory level initialization files

In Integration/Test, each suite (no matter how low level) could be called from a testplan.

Testplans can start higher-level suites, but then sub-suite execution ordering depends on alphabet, which might motivate contributors to use number-prefixed suite names, which is discouraged.

Instead, we recommend to have a Resource defining commonly-used Setup and Teardown keywords.

Dependencies between tests

See the discussion on unit of testing.

In other words, frequently only "dependencies between suites" is to be avoided.

Gherkin style

Usually our tests are not abstract enough for this to work. But no harm in trying, I guess.

Appropriate abstraction level

Most of our tests do not place high value in this requirement, avoiding state hiding has higher priority.

But perhaps committers should look for missed abstraction opportunities in order to shift Integration/test culture towards more abstraction where possible.