Skip to content

Conversation

savil
Copy link
Collaborator

@savil savil commented Mar 22, 2023

Summary

The testscripts can be run 2x faster. Specifically, time reduces from 30 seconds to 16 seconds.

the problem is:

  • in RunTestscripts function of testrunner.go, we call t.Run() which calls testscript.Run
  • inside testscript.Run, the framework calls t.Run() with t.Parallel().

The old way would run testscripts within the same sub-directory in parallel, but now we run all the testscripts in parallel.

The reason for this is:
When the golang test framework is executing a sub-test via t.Run, it will start executing it until it hits a t.Parallel(). It will then pause this sub-test. It then starts the next sub-test. And so on. Finally, it will resume all paused sub-tests which had t.Parallel() and run them concurrently.

By injecting the extra t.Run for each sub-directory we are limiting the parallellism.

ref. https://engineering.mercari.com/en/blog/entry/20220408-how_to_use_t_parallel/

Downside
If two testscripts have the same name, they'll be harder to tell apart in the test framework's output. For example, if we have two testscripts called info.test.txt, then their output appears as:

    --- PASS: TestScripts/info.test (0.68s)
    --- PASS: TestScripts/info.test#01 (0.57s)

This is entirely due to how testscript sets the sub-test's name. I couldn't think of a clever way of overriding that other than "copy all testscripts into a temp-dir and rename them to <dir>_<subdir1>_<subdir2>_<scriptname>.test.txt.

This downside doesn't currently happen since all our testscripts happen to be uniquely named.

How was it tested?

> go clean -testcache
> time go test -v ./testscripts/...

new output:

--- PASS: TestScripts (0.00s)
    --- PASS: TestScripts/assert.test (0.06s)
    --- PASS: TestScripts/recommend.test (0.06s)
    --- PASS: TestScripts/dockerfile.test (0.09s)
    --- PASS: TestScripts/devcontainer.test (0.10s)
    --- PASS: TestScripts/info.test (0.75s)
    --- PASS: TestScripts/shellception.test (2.12s)
    --- PASS: TestScripts/args.test (1.55s)
    --- PASS: TestScripts/unfree.test (3.08s)
    --- PASS: TestScripts/default_test_env.test (0.03s)
    --- PASS: TestScripts/empty.test (0.06s)
    --- PASS: TestScripts/shellenv.test (3.51s)
    --- PASS: TestScripts/path.test (4.21s)
    --- PASS: TestScripts/install_hello.test (3.03s)
    --- PASS: TestScripts/script.test (5.40s)
    --- PASS: TestScripts/rm.test (5.57s)
    --- PASS: TestScripts/php.test (8.58s)
    --- PASS: TestScripts/env.test (15.47s)
PASS
ok      go.jetpack.io/devbox/testscripts        15.911s

old output:

--- PASS: TestScripts (30.42s)
    --- PASS: TestScripts/assert (0.00s)
        --- PASS: TestScripts/assert/assert.test (0.02s)
    --- PASS: TestScripts/basic (0.00s)
        --- PASS: TestScripts/basic/default_test_env.test (0.02s)
        --- PASS: TestScripts/basic/install_hello.test (2.51s)
    --- PASS: TestScripts/generate (0.00s)
        --- PASS: TestScripts/generate/dockerfile.test (0.05s)
        --- PASS: TestScripts/generate/devcontainer.test (0.05s)
    --- PASS: TestScripts/info (0.00s)
        --- PASS: TestScripts/info/info.test (0.38s)
    --- PASS: TestScripts/init (0.00s)
        --- PASS: TestScripts/init/recommend.test (0.03s)
        --- PASS: TestScripts/init/empty.test (0.04s)
    --- PASS: TestScripts/languages (0.00s)
        --- PASS: TestScripts/languages/php.test (6.17s)
    --- PASS: TestScripts/packages (0.00s)
        --- PASS: TestScripts/packages/unfree.test (1.56s)
    --- PASS: TestScripts/rm (0.00s)
        --- PASS: TestScripts/rm/rm.test (4.33s)
    --- PASS: TestScripts/run (0.00s)
        --- PASS: TestScripts/run/args.test (1.27s)
        --- PASS: TestScripts/run/shellception.test (1.27s)
        --- PASS: TestScripts/run/path.test (2.94s)
        --- PASS: TestScripts/run/script.test (3.67s)
        --- PASS: TestScripts/run/env.test (13.50s)
    --- PASS: TestScripts/shell (0.00s)
        --- PASS: TestScripts/shell/shellenv.test (1.87s)
PASS
ok      go.jetpack.io/devbox/testscripts        30.781s

Copy link
Collaborator Author

savil commented Mar 22, 2023

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@savil savil requested review from loreto and gcurtis March 22, 2023 23:49
@savil savil marked this pull request as ready for review March 22, 2023 23:50
@savil savil requested a review from mikeland73 March 22, 2023 23:59
Copy link
Collaborator

@gcurtis gcurtis left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for figuring this out.

Does the test name still have the directory (or some other identifier) in it so we can tell which test failed?

@savil
Copy link
Collaborator Author

savil commented Mar 23, 2023

@gcurtis unfortunately, the name no longer includes the directory. This is what I discuss in the "downside" in the PR summary

@savil
Copy link
Collaborator Author

savil commented Mar 23, 2023

I'm going to land but we can iterate to improve the test name if folks have better ideas.

@savil savil merged commit ec26cb7 into main Mar 23, 2023
@savil savil deleted the savil/testscripts-parallel branch March 23, 2023 01:38
@savil
Copy link
Collaborator Author

savil commented Mar 23, 2023

We can currently tell which test failed since our test scripts are uniquely named.

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

Successfully merging this pull request may close these issues.

2 participants