Review feature files for adherence to BDD principles

[DPassarelli]

Problem

Feature files (Gherikin) should follow BDD principles, namely:

• Scenarios should read like actual system documentation, not just test specifications.
• Scenarios should have specific, real-world details, but only those directly related to the behavior under test (others should be generalized or abstracted to avoid breaking tests when unrelated behavior changes).
• Scenarios should only test behaviors, not implementations ("black box" as opposed to "white box"). There may be one or two exceptions to this rule, but they must be defensible.
• Avoid including Given statements that only exist to "set up plumbing". We should not muddle external specifications with internal needs.
• The When clause should only describe a single action (not a sequence of events).
• Each test should be focused around a single rule or outcome. Avoid "nice to haves" and only stick to "need to haves". Trim And statements wherever their exclusion would not affect the outcome of the test.
• There should be sufficient scenarios to cover both success and failure paths for each feature.
• Each scenario should be no more than 5 lines long, but ideally in the range of 3-4. Prefer a larger number of smaller, tightly focused scenarios rather than a smaller number of large, expansive scenarios.

Example 1

Feature: Compare directories

  In order to understand what a sync will do before committing to it,
  I want to see which files differ and what action would be taken.

  Background:
    Given a local directory containing these files:
      """
      src/main.go
      src/parser.go
      README.md
      LICENSE
      .gitignore
      """

Why this is good

Having an opening statement that reads like a user story is excellent. It gives all of the following scenarios a reason for existence and context. Having a Background that provides a common fixture setup is appropriate and reduces noise in each individual scenario.

Actions to take:

1. Ensure that all feature files include "In order to.../I want..." verbiage at the top of each file.
2. Ensure that all scenarios within a feature file are, in fact, strongly tied to the opening story. If not, separate them into a different feature and create a new story that is more appropriate for them.

What could be improved

The feature is called "Compare directories", but the story implies this is more about what a person sees and is told about the system under test. "Report differences between local and remote directories" might be a better title. compare is what the code is doing internally, but report is what is revealed externally.

Example 2

  Scenario: One of the files is different
    Given that all of the files are identical between local and remote
    And   that the file "README.md" has been changed locally
    When  I run "csync ./project user@host:/project"
    Then  the reported actions should be:
      | action | path      |
      | update | README.md |
    And   the reported change count should be 1

Why this is good

The use of a Data Table is good because it captures a set of information in a single Then clause (as opposed to having multiple Ands in other scenarios when there is more than one reported change expected).

What could be improved

Given that all of the files are identical between local and remote and And that the file "README.md" has been changed locally are contradictory statements. They can't both be true at the same time. Only the second statement is appropriate for this scenario, so the first one should be removed. If it exists merely for internal plumbing, then that plumbing should be made the default internal setup logic.

In the Then clause, the Data Table strongly implies the "reported change" count. It would be completely unexpected for that number not to match. Additionally, the implementation is being called out here: there are reported actions and a reported change count in the code itself. Prefer separating these into different scenarios, or dropping one of the assertions altogether (from the scenario; it can still be checked in the code-behind as part of "the reported actions should be").

Actions to take:

1. Ensure that Given clauses do not exist merely to provide internal plumbing. If internal plumbing is necessary for all scenarios, provide it in the default fixture setup code. It is okay if one of the Given statements (for example, Given that all of the files are identical between local and remote) ends up being a no-op.
2. Avoid contradictory Given statements. Prefer only describing what is different or unique about the scenario to provide a reason for its inclusion.
3. Avoid mapping Then clauses directly to implementation. If a single Then can reasonably be used to understand more than one implementation detail, prefer that over multiple ones that "expose the plumbing".

Example 3

  Scenario: A source that looks like an rsync option is treated as a path
    # Security regression guard. rsyncArgs puts `--` before the operands, so an
    # option-looking source (here `-e`, rsync's remote-shell flag) reaches rsync
    # as a path. That path doesn't exist, so rsync errors and csync exits
    # non-zero. Delete the `--` and rsync would honor `-e` and exit 0 — flipping
    # this red. This asserts the guard's *behavior*, not its implementation.
    # See SECURITY.md.
    When  I run "csync -e ./project"
    Then  csync should return a non-zero exit code

Why this is good

Security regressions are very important to guard against! Features should definitely include scenarios that exercise failure paths as much as success paths.

What could be improved

This is not entirely germane to the "Report differences" theme that the other scenarios are tied to. Consider moving this into a separate feature to make it easier to discover (perhaps "Guard against invalid input").

What success looks like

Review all features according to the robustness criteria above and flag anything of concern for further review.

Although scenarios are the canonical representation of system behavior, is there sufficient information to be able to quickly deduce what the major flows through the system look like from an end-user perspective? Would additional documentation be needed to provide this perspective quickly and clearly?