Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

testscript: test custom conditions #175

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
29 changes: 29 additions & 0 deletions testscript/doc.go
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,35 @@ when testing.Short() is false.

Additional conditions can be added by passing a function to Params.Condition.
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe worth mentioning this:

The function will only be called when all built-in conditions have been checked for.

Copy link
Author

Choose a reason for hiding this comment

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

Done


An example:
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for this to be a separate paragraph, IMO. You can attach it to the previous paragraph.

Copy link
Author

Choose a reason for hiding this comment

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

removed


Condition: func(cond string) (bool, error) {
// Assume condition name and args are separated by colon (":")
Copy link
Owner

Choose a reason for hiding this comment

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

I think that parsing the condition here is somewhat overkill tbh, and makes the example a bit longer than it needs to be. I'm pretty sure that people writing conditions are up to the task of extrapolating from something a bit simpler.

How about just something like:

Condition: func(cond string) (bool, error) {
    // Note: conditions can be more complex than this (for example [exists:/some/file]).
    if cond != "someCondition" {
        return false, fmt.Errorf("unknown condition")
    }
    // someCondition is a place-holder for any conditional logic you might want.
    return someCondition(), nil
}

?

Copy link
Author

Choose a reason for hiding this comment

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

I can do that, if everyone agrees. However, I wanted the example to include some text manipulation logic, because I was baffled when I first tried to define a condition. What was unclear to me was that custom commands are defined as a map of functions, while custom conditions are just one function for all.
I think the current example will save users some grief.

Copy link
Contributor

Choose a reason for hiding this comment

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

For what it's worth, I also feel this is likely to confuse people. But perhaps what this is really telling us is that the API should be a map of condition names to func(string) (bool, error). What do you think?

args := strings.Split(cond, ":")
// Empty condition is already managed in testscript.run()
name := args[0]
switch name {
case "is_upper":
if len(args) < 2 {
return false, fmt.Errorf("syntax: [is_upper:word]")
}
return strings.ToUpper(args[1]) == args[1], nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

to make the example with an argument a bit more realistic, how about a variant of exists? Like the built-in command, but being useful as a condition, like [exists:foo/bar] mycmd foo/bar.

case "exists":
  _, err := os.Lstat(args[1])
  return err == nil, nil

Copy link
Author

Choose a reason for hiding this comment

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

Changed the example

case "always_true":
return true, nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

similar to my other comment, I'd like this to be slightly more realistic: in practice I don't think anyone would write an "always true" condition :)

How about one that's tied to an env var? For example, I've used "slow" or "long" env vars to tell go test to do more extensive testing, kinda like the opposite of go test -short. So you could have a [long] that checks os.Getenv("TEST_LONG") == "true", for instance.

Copy link
Author

Choose a reason for hiding this comment

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

changed the example

default:
return false, fmt.Errorf("unrecognized condition %s", name)
}
},

With the conditions so defined, you can use them as follows:

env upper_word=ABCD
env lower_word=abcd
# the first two conditions will allow the 'echo' command to run. The third one will go silent.
[always_true] exec echo 'this is true'
[is_upper:$upper_word] exec echo 'found an uppercase word'
[is_upper:$lower_word] exec echo 'found an uppercase word'

The predefined commands are:

- cd dir
Expand Down
67 changes: 67 additions & 0 deletions testscript/testdata/custom_conditions.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
[!exec:echo] skip
Copy link
Owner

Choose a reason for hiding this comment

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

It would be nice to have a test that for the custom condition function returning an error, but that might be a bit awkward to do.

Copy link
Author

Choose a reason for hiding this comment

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

I couldn't find a way to do that.

Copy link
Author

@datacharmer datacharmer Aug 5, 2022

Choose a reason for hiding this comment

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

UPDATE: I found a way to test the errors (see testdata/custom_conditions_errors.txt).
Admittedly, it's a hack. It may not be resilient enough to certify future code update.


env upper_word=ABCD
env lower_word=abcd

[always_true] exec echo 'this is true'
stdout 'this is true'

exec echo ''

[!always_true] exec echo 'this is true'
! stdout .

exec echo ''

[always_false] exec echo 'this is false'
! stdout .

exec echo ''

[!always_false] exec echo 'this is false'
stdout 'this is false'

exec echo ''

[is_upper:ABCD] exec echo 'it is upper'
stdout 'it is upper'

exec echo ''

[is_upper:$upper_word] exec echo 'it is again upper'
stdout 'it is again upper'

exec echo ''

[is_upper:abcd] exec echo 'it is upper'
! stdout .

exec echo ''

[is_upper:$lower_word] exec echo 'it is lower'
! stdout .

exec echo ''

[!is_upper:ABCD] exec echo 'it is upper'
! stdout .

exec echo ''

[is_lower:abcd] exec echo 'it is lower'
stdout 'it is lower'

exec echo ''

[is_lower:$lower_word] exec echo 'it is again lower'
stdout 'it is again lower'

exec echo ''

[is_lower:ABCD] exec echo 'it is lower'
! stdout .

exec echo ''

[!is_lower:$lower_word] exec echo 'it is lower'
! stdout .
24 changes: 24 additions & 0 deletions testscript/testscript_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,30 @@ func TestScripts(t *testing.T) {
}
},
},
Condition: func(cond string) (bool, error) {
// Assume condition name and args are separated by colon (":")
args := strings.Split(cond, ":")
// Empty condition is already managed in testscript.run()
name := args[0]
switch name {
case "is_upper":
if len(args) < 2 {
return false, fmt.Errorf("syntax: [is_upper:word]")
}
return strings.ToUpper(args[1]) == args[1], nil
case "is_lower":
if len(args) < 2 {
return false, fmt.Errorf("syntax: [is_lower:word]")
}
return strings.ToLower(args[1]) == args[1], nil
case "always_true":
return true, nil
case "always_false":
return false, nil
default:
return false, fmt.Errorf("unrecognized condition %s", name)
}
},
Setup: func(env *Env) error {
infos, err := ioutil.ReadDir(env.WorkDir)
if err != nil {
Expand Down