APT-697 - fix tests for react-testing-library (#62)

* use robloxdev-cli for scripts

* upgrade foreman tools

* fix some bizarre subtleties of asynchronicity

* remove more no-op promise wrappers

* Simplify and dodge some issues with promise API details; needs further consideration

* upgrade a number of relevant dependencies

* fix lint

* stylua lint

* appease a few luau lints

* unfocus test

* fix string matching

* Introduce targeted wait to insure full cleanup

* a whole bunch of CI touchup

* clean up bad job definition

* one more try, was still referring to redundant job

* revert some misguided changes to the async wrapping and unmount logic

* introduce workaround to fix hanging tests

* Fix some type issues

* For now, point to the in-progress DTL changes

* fix a skipped test (which was broken) to try to bump coverage

* for now, lowering coverage gate. act-compat sees little coverage because we don't actually have to worry about old React 16.x versions floating around

* update dependency

* Run tests with mock scheduler on and off for added validation

* Add short-circuited real delay for use within test logic

Author: RoFlection Bot

bin/ci.sh

@@ -8,8 +8,17 @@ find Packages/_Index -name "*.robloxrc" | xargs rm -f
 
 echo "Run static analysis"
 selene src
-roblox-cli analyze analyze.project.json
+robloxdev-cli analyze analyze.project.json
 stylua -c src
 
 echo "Run tests"
-roblox-cli run --load.place tests.project.json --run bin/spec.lua --lua.globals=__DEV__=true --fastFlags.allOnLuau --fastFlags.overrides EnableLoadModule=true --fs.read=$PWD --load.asRobloxScript --headlessRenderer 1 --virtualInput 1
+robloxdev-cli run --load.place tests.project.json --run bin/spec.lua \
+    --lua.globals=__DEV__=true \
+    --fastFlags.allOnLuau --fastFlags.overrides EnableLoadModule=true \
+    --fs.read=$PWD --load.asRobloxScript --headlessRenderer 1 --virtualInput 1
+
+echo "Run tests with mock scheduler"
+robloxdev-cli run --load.place tests.project.json --run bin/spec.lua \
+    --lua.globals=__DEV__=true --lua.globals=__ROACT_17_MOCK_SCHEDULER__=true \
+    --fastFlags.allOnLuau --fastFlags.overrides EnableLoadModule=true \
+    --fs.read=$PWD --load.asRobloxScript --headlessRenderer 1 --virtualInput 1

foreman.toml

@@ -1,5 +1,6 @@
 [tools]
 rojo = { source = "Roblox/rojo-rbx-rojo", version = "7.2.1" }
 rotrieve = { source = "roblox/rotriever", version = "0.5.4" }
-selene = { source = "Roblox/Kampfkarren-selene", version = "0.20.0" }
+selene = { source = "Roblox/Kampfkarren-selene", version = "=0.26.1" }
 stylua = { source = "Roblox/JohnnyMorganz-StyLua", version = "=0.14.3" }
+rbx-aged-cli = { source = "Roblox/rbx-aged-tool", version = "=5.7.9" }

rotriever.toml

@@ -11,7 +11,7 @@ files = ["*", "!**/__tests__/**"]
 [dependencies]
 LuauPolyfill = "github.com/roblox/luau-polyfill@1"
 Promise = "github.com/roblox/roblox-lua-promise@3.3.0"
-DomTestingLibrary = "github.com/roblox/dom-testing-library-lua@8.14.0"
+DomTestingLibrary = "github.com/roblox/dom-testing-library-lua@8.14.2"
 React = "github.com/roblox/roact-alignment@17.0.1-rc.16"
 ReactRoblox = "github.com/roblox/roact-alignment@17.0.1-rc.16"
 Scheduler = "github.com/roblox/roact-alignment@17.0.1-rc.16"

src/__tests__/act.spec.lua

@@ -6,8 +6,6 @@ local expect = JestGlobals.expect
 local test = JestGlobals.test
 local jest = JestGlobals.jest
 
-local Promise = require(Packages.Promise)
-
 local React = require(Packages.React)
 local ParentModule = require(script.Parent.Parent)
 local render = ParentModule.render
@@ -25,13 +23,9 @@ test("render calls useEffect immediately", function()
 end)
 
 test("findByTestId returns the element", function()
-	return Promise.resolve()
-		:andThen(function()
-			local ref = React.createRef()
-			render(React.createElement("Frame", { ref = ref, [React.Tag] = "data-testid=foo" }))
-			expect(screen.findByTestId("foo"):expect()).toBe(ref.current)
-		end)
-		:expect()
+	local ref = React.createRef()
+	render(React.createElement("Frame", { ref = ref, [React.Tag] = "data-testid=foo" }))
+	expect(screen.findByTestId("foo"):expect()).toBe(ref.current)
 end)
 
 test("fireEvent triggers useEffect calls", function()

src/__tests__/cleanup.spec.lua

@@ -36,6 +36,7 @@ test("cleans up the document", function()
 	end
 
 	function Test:render()
+		-- selene: allow(roblox_incorrect_roact_usage)
 		return React.createElement("Frame", { Name = divId })
 	end
 

src/__tests__/end-to-end.spec.lua

@@ -89,38 +89,26 @@ Array.forEach({
 		end)
 
 		test("waitForElementToBeRemoved", function()
-			return Promise.resolve()
-				:andThen(function()
-					render(React.createElement(ComponentWithLoader, nil))
-					local function loading()
-						return screen.getByText("Loading...")
-					end
-					waitForElementToBeRemoved(loading):expect()
-					expect(screen.getByTestId("message")).toHaveTextContent(RegExp("Hello World"))
-				end)
-				:expect()
+			render(React.createElement(ComponentWithLoader, nil))
+			local function loading()
+				return screen.getByText("Loading...")
+			end
+			waitForElementToBeRemoved(loading):expect()
+			expect(screen.getByTestId("message")).toHaveTextContent(RegExp("Hello World"))
 		end)
 
 		test("waitFor", function()
-			return Promise.resolve()
-				:andThen(function()
-					render(React.createElement(ComponentWithLoader, nil))
-					local function message()
-						return screen.getByText(RegExp("Loaded this message:"))
-					end
-					waitFor(message):expect()
-					expect(screen.getByTestId("message")).toHaveTextContent(RegExp("Hello World"))
-				end)
-				:expect()
+			render(React.createElement(ComponentWithLoader, nil))
+			local function message()
+				return screen.getByText(RegExp("Loaded this message:"))
+			end
+			waitFor(message):expect()
+			expect(screen.getByTestId("message")).toHaveTextContent(RegExp("Hello World"))
 		end)
 
 		test("findBy", function()
-			return Promise.resolve()
-				:andThen(function()
-					render(React.createElement(ComponentWithLoader, nil))
-					expect((screen.findByTestId("message"):expect())).toHaveTextContent(RegExp("Hello World"))
-				end)
-				:expect()
+			render(React.createElement(ComponentWithLoader, nil))
+			expect((screen.findByTestId("message"):expect())).toHaveTextContent(RegExp("Hello World"))
 		end)
 	end)
 end)

src/__tests__/render.spec.lua

@@ -107,11 +107,11 @@ test("renders options.wrapper around node", function()
 end)
 
 -- ROBLOX FIXME: useEffect is triggered before unmount
-test.skip("flushes useEffect cleanup functions sync on unmount()", function()
-	local spy = jest.fn()
+test("flushes useEffect cleanup functions sync on unmount()", function()
+	local spy, spyFn = jest.fn()
 	local function Component()
 		React.useEffect(function()
-			spy()
+			return spyFn
 		end, {})
 		return nil
 	end

src/act-compat.lua

@@ -48,7 +48,12 @@ local function asyncAct(cb)
 								args[
 									1 --[[ ROBLOX adaptation: added 1 to array index ]]
 								],
-								"Warning: Do not await the result of calling ReactTestUtils.act"
+								-- ROBLOX deviation: in upstream, this error is accounting for a specific react version
+								-- (16.8.6); for us, it needs to account for the exact error version in our ported
+								-- ReactTestUtilsPublicAct
+								"Do not await the result of calling act(...) with sync logic, it is not a Promise.",
+								1,
+								true
 							)
 							== 1
 					then
@@ -58,7 +63,9 @@ local function asyncAct(cb)
 						firstArgIsString
 						and string.find(
 								args[1],
-								"Warning: The callback passed to ReactTestUtils.act(...) function must not return anything"
+								"Warning: The callback passed to ReactTestUtils.act(...) function must not return anything",
+								1,
+								true
 							)
 							== 1
 					then

src/jsHelpers/react-dom/test-utils/ReactTestUtilsPublicAct.lua

@@ -41,6 +41,10 @@ local batchedUpdates = ReactRoblox.unstable_batchedUpdates
 
 local IsSomeRendererActing = ReactSharedInternals.IsSomeRendererActing
 
+-- ROBLOX deviation: use realDelay in place of enqueueTask as a fallback to
+-- avoid trouble caused by fake timers
+local realDelay = require(script.Parent["realTaskDelay.roblox.global"])
+
 -- This is the public version of `ReactTestUtils.act`. It is implemented in
 -- "userspace" (i.e. not the reconciler), so that it doesn't add to the
 -- production bundle size.
@@ -64,7 +68,24 @@ end
 local function flushWorkAndMicroTasks(onDone: (err: any?) -> ())
 	local ok, err = pcall(function()
 		flushWork()
-		enqueueTask(function()
+		-- ROBLOX deviation START: We need to work around fake timers here in a
+		-- way that upstream would in `enqueueTask`'s logic. See the upstream
+		-- implementation here:
+		-- https://github.com/facebook/react/blob/v17.0.1/packages/shared/enqueueTask.js#L16-L22
+		local enqueueTaskWithRelevantTimers: (() -> ()) -> () = enqueueTask
+		if type(task.delay) == "table" then
+			-- ROBLOX TODO: the upstream implementation referenced above
+			-- ultimately uses the _real_ implementation of `setImmediate`,
+			-- which does wait for the next tick in the event loop. In our
+			-- implementation, fake timers fundamentally block access to
+			-- `task.delay`, which is our closest equivalent. Instead, it should
+			-- be sufficient to ignore the engine frame boundary and use
+			-- `task.defer` instead, which is unaffected by fake timers
+			enqueueTaskWithRelevantTimers = function(fn)
+				return realDelay(0, fn)
+			end
+		end
+		enqueueTaskWithRelevantTimers(function()
 			if flushWork() then
 				flushWorkAndMicroTasks(onDone)
 			else
@@ -94,12 +115,10 @@ local function act(callback: (() -> Thenable<any>) | () -> ())
 		end
 	end
 	local previousActingUpdatesScopeDepth = actingUpdatesScopeDepth
-	local previousIsSomeRendererActing
-	local previousIsThisRendererActing
 	actingUpdatesScopeDepth += 1
 
-	previousIsSomeRendererActing = IsSomeRendererActing.current
-	previousIsThisRendererActing = IsThisRendererActing.current
+	local previousIsSomeRendererActing = IsSomeRendererActing.current
+	local previousIsThisRendererActing = IsThisRendererActing.current
 	IsSomeRendererActing.current = true
 	IsThisRendererActing.current = true
 

src/jsHelpers/react-dom/test-utils/realTaskDelay.roblox.global.lua

@@ -0,0 +1,3 @@
+-- Returns a reference to the _real_ task.delay function, to be used by RTL
+-- internals where `enqueueTask` isn't quite aligned
+return task.delay