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

Refactor implement step runner #48

Merged
merged 20 commits into from
Dec 15, 2021
Merged

Conversation

rihter007
Copy link
Contributor

Move all logic that manages running of a TestStep in a separate TestRunner.
This refactoring is needed to make retries of steps look better.

Special thanks to @rojer for a large number of unit tests :)

@rihter007 rihter007 force-pushed the refactor_implement_step_runner branch from a7f762a to 53e21d1 Compare November 25, 2021 16:30
@rihter007
Copy link
Contributor Author

StepRunner logic is covered by TestRunner unit tests.
If this approach gets approved, I will add some unit tests for StepRunner as well

@rihter007 rihter007 force-pushed the refactor_implement_step_runner branch 2 times, most recently from 9c362f1 to c5098b3 Compare November 25, 2021 23:02
@rihter007
Copy link
Contributor Author

Cool, tests found existing race-condition, in case of an error, we may not "switch" to result processing branch in waitStepRunners method

time="2021-11-25T23:30:03Z" level=debug msg="monitor: active" file="test_runner.go:611"
time="2021-11-25T23:30:03Z" level=debug msg="monitor: starting at step [#0 Step 1]" file="test_runner.go:616"
time="2021-11-25T23:30:03Z" level=debug msg="[Target{ID: "T1"} 0 init false ]: target handler active" file="test_runner.go:424" target=T1
time="2021-11-25T23:30:03Z" level=debug msg="monitor pass 1: current step [#0 Step 1]" file="test_runner.go:625"
time="2021-11-25T23:30:03Z" level=debug msg="monitor pass 1: [#0 Step 1]: [Target{ID: "T1"} 0 init false ]" file="test_runner.go:629"
time="2021-11-25T23:30:03Z" level=debug msg="monitor pass 1: [#0 Step 1]: not all targets injected yet ([Target{ID: "T1"} 0 init false ])" file="test_runner.go:634"
time="2021-11-25T23:30:03Z" level=debug msg="[Target{ID: "T1"} 0 begin false ]: injecting into [#0 Step 1]" file="test_runner.go:350" target=T1
time="2021-11-25T23:30:03Z" level=debug msg="monitor pass 2: current step [#0 Step 1]" file="test_runner.go:625"
time="2021-11-25T23:30:03Z" level=debug msg="monitor pass 2: [#0 Step 1]: [Target{ID: "T1"} 0 run false ]" file="test_runner.go:629"
time="2021-11-25T23:30:03Z" level=info msg="Obtained '{Target{ID: "T1"} }' for target 'T1'" file="step_runner.go:178" step_index=0 step_label="Step 1"
time="2021-11-25T23:30:03Z" level=debug msg="monitor pass 2: [#0 Step 1]: no more targets, closing input channel" file="test_runner.go:648"
time="2021-11-25T23:30:03Z" level=debug msg="Input channel was closed" file="step_runner.go:145" step_index=0 step_label="Step 1"
time="2021-11-25T23:30:03Z" level=debug msg="TestStep finished '', rs " file="step_runner.go:230" step_index=0 step_label="Step 1"
time="2021-11-25T23:30:03Z" level=debug msg="monitor: waiting for targets to finish" file="test_runner.go:653"
time="2021-11-25T23:30:03Z" level=debug msg="monitor pass 3: [Target{ID: "T1"} 0 run false ]" file="test_runner.go:661"
time="2021-11-25T23:30:03Z" level=error msg="err: test step Step 1 closed output channels (api violation)" file="step_runner.go:249" step_index=0 step_label="Step 1"
time="2021-11-25T23:30:03Z" level=debug msg="output channel closed" file="step_runner.go:210" step_index=0 step_label="Step 1"
time="2021-11-25T23:30:03Z" level=debug msg="Running loop finished" file="step_runner.go:96" step_index=0 step_label="Step 1"
time="2021-11-25T23:30:03Z" level=debug msg="[#0 Step 1]: result for [Target{ID: "T1"} 0 result_pending false ]: " file="test_runner.go:569" step_index=0 step_label="Step 1"
time="2021-11-25T23:30:03Z" level=error msg="err: test step Step 1 closed output channels (api violation)" file="test_runner.go:515" step_index=0 step_label="Step 1"
time="2021-11-25T23:30:03Z" level=debug msg="monitor: finished, test step Step 1 closed output channels (api violation)" file="test_runner.go:689"
time="2021-11-25T23:30:03Z" level=error msg="monitor returned error: "test step Step 1 closed output channels (api violation)", canceling" file="test_runner.go:216"
time="2021-11-25T23:30:03Z" level=debug msg="waiting for step runners to finish" file="test_runner.go:306"
time="2021-11-25T23:30:03Z" level=debug msg="waiting for target handlers to finish" file="test_runner.go:228"
time="2021-11-25T23:30:03Z" level=debug msg="[Target{ID: "T1"} 0 result_pending false ]: canceled 2" file="test_runner.go:414" target=T1
time="2021-11-25T23:30:03Z" level=error msg="context canceled" file="test_runner.go:475" step_index=0 step_label="Step 1"
time="2021-11-25T23:30:03Z" level=debug msg="[Target{ID: "T1"} 0 result_pending false ]: canceled 1" file="test_runner.go:480" step_index=0 step_label="Step 1"
time="2021-11-25T23:30:03Z" level=debug msg="[Target{ID: "T1"} 0 result_pending false ]: target handler finished" file="test_runner.go:499" target=T1
time="2021-11-25T23:30:03Z" level=debug msg="leaving, err test step Step 1 closed output channels (api violation), target states:" file="test_runner.go:240"
time="2021-11-25T23:30:03Z" level=debug msg=" 0 [Target{ID: "T1"} 0 result_pending true ] test step Step 1 closed output channels (api violation) false" file="test_runner.go:257"
time="2021-11-25T23:30:03Z" level=debug msg="- 0 in flight, ok to resume? false" file="test_runner.go:259"
time="2021-11-25T23:30:03Z" level=debug msg="Output channel closed" file="step_runner.go:163" step_index=0 step_label="Step 1"
time="2021-11-25T23:30:03Z" level=debug msg="step states:" file="test_runner.go:260"
time="2021-11-25T23:30:03Z" level=debug msg="Reading loop finished" file="step_runner.go:102" step_index=0 step_label="Step 1"
time="2021-11-25T23:30:03Z" level=debug msg="StepRunner finished" file="step_runner.go:89" step_index=0 step_label="Step 1"
time="2021-11-25T23:30:03Z" level=debug msg=" 0 [#0 Step 1] true true test step Step 1 closed output channels (api violation) " file="test_runner.go:262"
--- FAIL: TestTestRunnerSuite (2.23s)
--- FAIL: TestTestRunnerSuite/TestStepClosesChannels (0.01s)
test_runner_test.go:381:
Error Trace: test_runner_test.go:381
Error: Not equal:
expected: "\n{[1 1 SimpleTest 0 Step 1][Target{ID: "T1"} TargetIn]}\n{[1 1 SimpleTest 0 Step 1][Target{ID: "T1"} TargetOut]}\n"
actual : "\n{[1 1 SimpleTest 0 Step 1][Target{ID: "T1"} TargetIn]}\n"

        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -2,3 +2,2 @@
        	            	 {[1 1 SimpleTest 0 Step 1][Target{ID: "T1"} TargetIn]}
        	            	-{[1 1 SimpleTest 0 Step 1][Target{ID: "T1"} TargetOut]}
        	            	 
        	Test:       	TestTestRunnerSuite/TestStepClosesChannels

FAIL
FAIL github.com/linuxboot/contest/pkg/runner 7.078s
FAIL
1

Another test fails because it lasted 7 seconds instead of 5... The test checks exec plugin that launches actual ssh stuff. Will extend timeout.

@rojer
Copy link
Contributor

rojer commented Nov 26, 2021

Special thanks to @rojer for a large number of unit tests :)

you are welcome :)
i really tried to test it well after rewrite. of course, i did miss one case so it ended up blowing up anyway on first attempt...
but hopefully these tests will now help you.

@codecov-commenter
Copy link

codecov-commenter commented Nov 26, 2021

Codecov Report

Merging #48 (62d049c) into main (027c811) will increase coverage by 0.22%.
The diff coverage is 88.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #48      +/-   ##
==========================================
+ Coverage   66.25%   66.47%   +0.22%     
==========================================
  Files         156      157       +1     
  Lines        9346     9457     +111     
==========================================
+ Hits         6192     6287      +95     
- Misses       2488     2501      +13     
- Partials      666      669       +3     
Flag Coverage Δ
e2e 51.08% <71.53%> (+0.61%) ⬆️
integration 58.18% <79.56%> (+0.79%) ⬆️
unittests 48.03% <88.51%> (+0.81%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/cerrors/cerrors.go 25.00% <0.00%> (+25.00%) ⬆️
pkg/runner/step_runner.go 89.37% <89.37%> (ø)
pkg/runner/test_runner.go 90.40% <90.90%> (-1.82%) ⬇️
plugins/teststeps/waitport/waitport.go 68.93% <0.00%> (ø)
pkg/xcontext/context.go 65.38% <0.00%> (+0.96%) ⬆️
pkg/xcontext/event_handler.go 88.04% <0.00%> (+1.63%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 027c811...62d049c. Read the comment docs.

@rihter007 rihter007 force-pushed the refactor_implement_step_runner branch 2 times, most recently from 40d3afd to e236ec9 Compare November 26, 2021 17:20
pkg/runner/test_runner.go Show resolved Hide resolved
pkg/runner/step_runner.go Outdated Show resolved Hide resolved
pkg/runner/step_runner.go Outdated Show resolved Hide resolved
pkg/runner/test_runner.go Outdated Show resolved Hide resolved
pkg/runner/test_runner.go Outdated Show resolved Hide resolved
pkg/runner/step_runner.go Outdated Show resolved Hide resolved
pkg/runner/step_runner.go Outdated Show resolved Hide resolved
pkg/runner/step_runner.go Outdated Show resolved Hide resolved
pkg/runner/step_runner.go Show resolved Hide resolved
pkg/runner/step_runner.go Outdated Show resolved Hide resolved
pkg/runner/step_runner.go Outdated Show resolved Hide resolved
tests/integ/plugins/testrunner_test.go Show resolved Hide resolved
@rihter007 rihter007 force-pushed the refactor_implement_step_runner branch from 2b60d09 to 0aa919e Compare December 5, 2021 22:43
@rihter007 rihter007 force-pushed the refactor_implement_step_runner branch from 0aa919e to 462f278 Compare December 5, 2021 22:45
@rihter007 rihter007 requested a review from mimir-d December 5, 2021 23:00
pkg/runner/step_runner.go Show resolved Hide resolved
pkg/cerrors/cerrors.go Show resolved Hide resolved
@rihter007 rihter007 requested a review from tfg13 December 6, 2021 14:12
Clear StepRunner.WaitResult() signature

Signed-off-by: Ilya <[email protected]>
Make StepRunner.Run() use channels for communicating results

Signed-off-by: Ilya <[email protected]>
@rihter007 rihter007 force-pushed the refactor_implement_step_runner branch from 6ee432d to 62d049c Compare December 14, 2021 23:57
@rihter007 rihter007 merged commit 0f755b4 into main Dec 15, 2021
@rihter007 rihter007 deleted the refactor_implement_step_runner branch December 15, 2021 19:15
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.

5 participants