Harden runner edge cases#113
Conversation
|
@sarahsturgeon - could you take a look at the runner edge-case hardening in this PR when you have a chance, or route it to the right reviewer? I can make any requested changes or add tests. Thanks! |
|
@sarahsturgeon Friendly reminder: we'd appreciate it if you could take a look at this pull request when you have a chance. |
A1steaksa
left a comment
A1steaksa
left a comment
There was a problem hiding this comment.
These seem like good changes. There are a few small things I've noted, but generally I think this is an improvement.
Thanks for taking the time to contribute to GLuaTest!
|
|
||
| cases = { | ||
| { | ||
| name = "Records sync beforeEach errors as failures", |
There was a problem hiding this comment.
Case name is difficult to understand unless you already know what the case is and does
There was a problem hiding this comment.
Renamed this case to make the expected behavior explicit: Fails the case when beforeEach errors. See lua/tests/gluatest/runner/test_case_runner.lua in efd921b.
| end | ||
| }, | ||
| { | ||
| name = "Records async beforeEach errors as failures", |
There was a problem hiding this comment.
Case name is difficult to understand unless you already know what the case is and does
There was a problem hiding this comment.
Renamed this case to make the async behavior explicit: Fails the async case when beforeEach errors. See lua/tests/gluatest/runner/test_case_runner.lua in efd921b.
| name = "Sends shared cases when the gluatest_client_enable ConVar is enabled", | ||
| func = function( state ) | ||
| local Loader = state.Loader | ||
| state.currentRunClientside = GLuaTest.RunClientsideConVar:GetBool() |
There was a problem hiding this comment.
Nit: This variable stores the prior RunClientsideConVar value, not the current value
There was a problem hiding this comment.
Renamed state.currentRunClientside to state.previousRunClientside throughout the loader self-test file in efd921b.
| assert( TypeID( subject ) == TYPE_FUNCTION, ".succeed expects a function" ) | ||
|
|
||
| local success = pcall( subject, unpack( args ) ) | ||
| local success = pcall( subject, unpack( args, 1, args.n ) ) |
There was a problem hiding this comment.
These additions to unpack throughout the PR strike me as unnecessary, given that the defaults for unpack should produce these same results
There was a problem hiding this comment.
Removed the unpack arity change and dropped the trailing-nil self-tests in efd921b.
|
|
||
| if success == true then | ||
| i.expected( "to error" ) | ||
| return |
There was a problem hiding this comment.
This if condition should be inverted to remove the case that just calls return to remove the "empty" code path
There was a problem hiding this comment.
Inverted the branch so errWith only handles the failing call path; successful calls now fall through in efd921b.
2 participants
Summary
Testing
git diff --check