Skip to content

Implement subtest sections in input files#11

Merged
knz merged 2 commits intocockroachdb:masterfrom
knz:20191128-subtests
Nov 29, 2019
Merged

Implement subtest sections in input files#11
knz merged 2 commits intocockroachdb:masterfrom
knz:20191128-subtests

Conversation

@knz
Copy link
Copy Markdown
Contributor

@knz knz commented Nov 28, 2019

Fixes #8.

This commit removes the one-subtest-per-directive logic and replaces
it by proper subtest support in the input DSL.

A sub-test begins with subtest <name> and ends with subtest end [<name>].

  • Sub-tests can be nested. However, the name of a nested sub-test must
    be prefixed by the name of the enclosing sub-test and a /.
  • The name is optional after end; if present, it must match the
    corresponding start directive.
  • Obviously "end" is not a valid name to start a subtest.

For example:

subtest foo

subtest foo/bar

subtest end foo/bar

subtest end

This will be used in cockroachdb/cockroach#42250.


This change is Reviewable

@knz knz requested review from RaduBerinde and tbg November 28, 2019 11:05
@knz
Copy link
Copy Markdown
Contributor Author

knz commented Nov 28, 2019

@RaduBerinde do you think we should require subtest end to mention the name of the corresponding start directive? (e.g. subtest end <name>?)

@knz knz force-pushed the 20191128-subtests branch from 40060c0 to d9e8321 Compare November 29, 2019 07:27
Copy link
Copy Markdown
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should require that duplication. But it should be allowed (if it isn't already) to follow up the directive with an arbitrary comment subtest end # foo

I'm a little worried folks will use subtest after subtest and that will be allowed, and maybe they'll add the subtest ends at the very end to fix the errors, not realizing they are all nested. I think it's a bit overkill to allow nested subtests, but if we want that we should require that the subtest name includes the parent (eg subtest foo/bar for subtest bar inside foo).

This commit removes the one-subtest-per-directive logic and replaces
it by proper subtest support in the input DSL.

A sub-test begins with `subtest <name>` and ends with `subtest end [<name>]`.

- Sub-tests can be nested. However, the name of a nested sub-test must
  be prefixed by the name of the enclosing sub-test and a `/`.
- The name is optional after `end`; if present, it must match the
  corresponding start directive.
- Obviously "`end`" is not a valid name to start a subtest.

For example:

```
subtest foo

subtest foo/bar

subtest end foo/bar

subtest end
```
@knz knz force-pushed the 20191128-subtests branch from d9e8321 to ba50173 Compare November 29, 2019 13:23
@knz
Copy link
Copy Markdown
Contributor Author

knz commented Nov 29, 2019

I don't think we should require that duplication. But it should be allowed (if it isn't already) to follow up the directive with an arbitrary comment subtest end # foo

It's not that easy to parse "comments until the end of a line" without a proper lexer, which is not there today. So instead I opted to let end accept an optional name, which must match if included.

[...] we should require that the subtest name includes the parent (eg subtest foo/bar for subtest bar inside foo).

Good idea. Done. It's also good to ease with filtering.

@knz knz merged commit 210cca0 into cockroachdb:master Nov 29, 2019
Copy link
Copy Markdown
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Re-consider running each directive in its own sub-test

2 participants