Added beforeAll, beforeEach, afterEach, afterAll lifecycle hooks for testing #57

benbrimeyer · 2020-01-15T23:16:08Z

The setup and teardown behavior of these hooks attempt to reach feature parity with jest.

Test nodes will be marked as failures if the beforeEach, beforeAll, or afterEach hook throws. Currently there is a limitation in this changelist that does not mark the nodes as failures if afterAll hook throws.

This pull request replaces the need for #39

…-lifecycle

AmaranthineCodices

Couple minor things I noticed

tests/lifecycleHooks.lua

AmaranthineCodices · 2020-01-16T02:20:59Z

lib/TestRunner.lua

-						errorMessage = nodeResult
+					for _, hook in pairs(lifecycleHooks:getAfterEachHooks()) do
+						if success then
+							success, errorMessage = runCallback(hook, true, "after each hook: ")


The message prefix here is inconsistent with the prefix for the other lifecycle hooks:

Suggested change

success, errorMessage = runCallback(hook, true, "after each hook: ")

success, errorMessage = runCallback(hook, true, "afterEach hook: ")

LPGhatguy

Just took a cursory glance for my evening reading, I'll dive in more at work tomorrow. 😄

Can you revert the changes to files that are just trailing line endings at the end of files? We can go back through the codebase at a later time to catch those and do some other reformatting work.

LPGhatguy · 2020-01-16T03:18:08Z

.gitignore

+/site
+/lua_install


Are you running hererocks locally? This is an acceptable change, it's just interesting that this is how you got your Lua install to work 😅

This was carry over from the previous pull request. I can remove it if needed.

LPGhatguy · 2020-01-16T03:21:45Z

.luacheckrc

+	},
+}
+
+std = "lua51+roblox+testez"


When we're testing TestEZ itself, we don't generally have access to TestEZ, so I'm not sure that adding it to our Luacheck config is correct here.

If I dont add it here, Tests/lifecycleHooks.lua wont pass luacheck since we are testing these globals

We should narrow down the scope of what files we inject these globals into, then. I don't want to make Luacheck think that the entire TestEZ codebase has access to these values, it mostly doesn't.

We've got two options I think:

Use Luacheck assertions on the functions that use these methods, if there's a small number of them.

Only apply the testez std for the tests folder or even the specific test that depends on these.

seems to be the most applicable, since this sounds like a problem of scoping.
We can create a new luacheckrc file for the tests directory that has the testez std. Would that work?

Edit: We could do 1. as well, there's about 4 tests in lifecycleHooks that use these functions at the moment.

We don't need to make a new luacheckrc file, just scope the rules using globs to only affect the files in the tests folder like we do in Roact:

https://github.com/Roblox/roact/blob/master/.luacheckrc

benbrimeyer · 2020-01-16T22:13:56Z

Thanks for the first round of feedback. All of it should be addressed now.

lib/TestRunner.lua

LPGhatguy

Took a more detailed look through this.

lib/LifecycleHooks.lua

LPGhatguy · 2020-01-17T22:59:41Z

lib/LifecycleHooks.lua

+		[TestEnum.NodeType.AfterAll] = self:_getAfterAllHooksUncalledAtCurrentLevel(planNode.children),
+		[TestEnum.NodeType.BeforeEach] = self:_getHooksOfType(planNode.children, TestEnum.NodeType.BeforeEach),
+		[TestEnum.NodeType.AfterEach] = self:_getHooksOfType(planNode.children, TestEnum.NodeType.AfterEach),
+	})


This is a mouthful!

lib/LifecycleHooks.lua

lib/TestRunner.lua

LPGhatguy · 2020-01-17T23:09:46Z

lib/TestRunner.lua

+					for _, hook in pairs(lifecycleHooks:getPendingBeforeHooks()) do
+						if success then
+							success, errorMessage = runCallback(hook, false, "beforeAll hook: ")
+						end


We don't run afterAll hooks in this block, but we run beforeAll here.

Why don't we process beforeAll above, before we enter this child loop?

We can do that, but if the beforeAll node fails I'm afraid I wont have access to the child nodes it was wrapping as easily

Also if we move beforeAll out, we wont be able to filter out describe blocks with beforeAlls with no It blocks very easily.

Do we have the same problem with afterAll not being filtered? I wonder if we should try to patch this up a little in TestEZ and then start fresh in the new test framework with lifecycle events in mind from the start.

LPGhatguy · 2020-01-17T23:10:19Z

lib/TestRunner.lua

@@ -110,7 +144,7 @@ function TestRunner.runPlanNode(session, planNode, tryStack, noXpcall)
 			end
 		elseif childPlanNode.type == TestEnum.NodeType.Describe or childPlanNode.type == TestEnum.NodeType.Try then
 			if childPlanNode.type == TestEnum.NodeType.Try then tryStack:push({isOk = true, failedNode = nil}) end
-			TestRunner.runPlanNode(session, childPlanNode, tryStack, childPlanNode.HACK_NO_XPCALL)
+			TestRunner.runPlanNode(session, childPlanNode, tryStack, lifecycleHooks, childPlanNode.HACK_NO_XPCALL)


This function's starting to get a lot of arguments. We're right at the threshold where refactoring it to take a table of arguments might be a good idea, but we can leave that out of this PR.

tests/lifecycleHooks.lua

LPGhatguy · 2020-01-17T23:12:25Z

tests/lifecycleHooks.lua

+
+	local results = TestEZ.TestRunner.runPlan(plan)
+	return results, lifecycleOrder
+end


This is a neat way to test this feature.

…y returns

LPGhatguy

LEt's merge to master!

inversion and others added 16 commits April 18, 2019 10:57

Rough implementation of lifecycle hooks (before/after (each))

e26ec2b

always run after hooks

6850da6

Merge branch 'master' of https://github.com/Roblox/testez into revive…

dc1ea45

…-lifecycle

tests are failing, but we have a new test now

1a4efbf

its functional with basic tests passing

3fac9b0

refactored lifecycle hooks to use runTestPlan helper function

bd3d2b8

update luacheckrc to accept testEZ globals

958c064

clean up lifecycleHooks.lua some more

4ab1007

beforeAll, beforeEach, and afterEach throwing will fail test node

1d584f0

fixed single quotes to double quotes

a3fbe15

reorganized LifecycleHooks.lua

2469f65

link to jest spec test

10d5182

formatting in TestRunner

ced703f

moves lifecycleHooks error tests to end

dfc44bd

Added entry to CHANGELOG.md

fa8e9c5

Cleans up formatting with single to double quotes

3f3f069

benbrimeyer requested review from LPGhatguy and amatosov-rbx January 15, 2020 23:18

AmaranthineCodices suggested changes Jan 16, 2020

View reviewed changes

LPGhatguy reviewed Jan 16, 2020

View reviewed changes

benbrimeyer added 3 commits January 16, 2020 14:07

comment on intentially missing afterAll

7db7e85

Consistent hook prefix

1cdd1b6

updated luacheckrc to focus testez globals on lifecycleHooks.lua only

c698915

benbrimeyer requested review from AmaranthineCodices and LPGhatguy January 16, 2020 22:13

remove extra new line at end of files

6810ffe

AmaranthineCodices approved these changes Jan 17, 2020

View reviewed changes

lib/TestRunner.lua Show resolved Hide resolved

amatosov-rbx approved these changes Jan 17, 2020

View reviewed changes

LPGhatguy suggested changes Jan 17, 2020

View reviewed changes

benbrimeyer added 3 commits January 17, 2020 17:03

remove stack from lifecycleHooks.lua

7730555

add runNode function in TestRunner to simplify statement and add earl…

960e2d7

…y returns

changed assertion into error for string formatting reasons

0e16e7b

LPGhatguy mentioned this pull request Jan 21, 2020

Rough implementation of lifecycle hooks (before/after (each)) #39

Closed

Merge branch 'master' into revive-lifecycle

a16bbfe

LPGhatguy approved these changes Jan 21, 2020

View reviewed changes

LPGhatguy merged commit 5fa229b into Roblox:master Jan 21, 2020

benbrimeyer deleted the revive-lifecycle branch January 21, 2020 22:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added beforeAll, beforeEach, afterEach, afterAll lifecycle hooks for testing #57

Added beforeAll, beforeEach, afterEach, afterAll lifecycle hooks for testing #57

benbrimeyer commented Jan 15, 2020 •

edited

Loading

AmaranthineCodices left a comment

AmaranthineCodices Jan 16, 2020

LPGhatguy left a comment

LPGhatguy Jan 16, 2020

benbrimeyer Jan 16, 2020

LPGhatguy Jan 16, 2020

benbrimeyer Jan 16, 2020

LPGhatguy Jan 16, 2020

benbrimeyer Jan 16, 2020 •

edited

Loading

LPGhatguy Jan 16, 2020

benbrimeyer commented Jan 16, 2020

LPGhatguy left a comment

LPGhatguy Jan 17, 2020

LPGhatguy Jan 17, 2020

benbrimeyer Jan 18, 2020

benbrimeyer Jan 18, 2020

LPGhatguy Jan 20, 2020

LPGhatguy Jan 17, 2020

LPGhatguy Jan 17, 2020

LPGhatguy left a comment

	success, errorMessage = runCallback(hook, true, "after each hook: ")
	success, errorMessage = runCallback(hook, true, "afterEach hook: ")

		/site
		/lua_install

Added beforeAll, beforeEach, afterEach, afterAll lifecycle hooks for testing #57

Added beforeAll, beforeEach, afterEach, afterAll lifecycle hooks for testing #57

Conversation

benbrimeyer commented Jan 15, 2020 • edited Loading

AmaranthineCodices left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LPGhatguy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benbrimeyer Jan 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benbrimeyer commented Jan 16, 2020

LPGhatguy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LPGhatguy left a comment

Choose a reason for hiding this comment

benbrimeyer commented Jan 15, 2020 •

edited

Loading

benbrimeyer Jan 16, 2020 •

edited

Loading