[CIS-108] Improve WebSocket tests #268

VojtaStavik · 2020-05-13T07:47:07Z

In this PR:

New Timer wrapper type for timers. This allows us to inject custom timers when running tests.
It's now possible to inject Timer.type to WebSocket.
Introduced VirtualTime for running timers in tests without actually waiting. The implementation of the run(_:) method is a little bit tricky. Please see VirtualTimeTests if you want to understand why it's done like this. Also, happy to see your suggestions and improvements.
Refactored existing WebSocket tests to use virtual time. I didn't refactor the tests for Event callbacks because we'll change this part soon and we'll add tests as a part of the refactoring.

#no_changelog

Stream-SDK-Bot · 2020-05-13T07:51:54Z

	1 Warning
⚠️	Big PR

Generated by 🚫 Danger

b-onc

🧠 🔥

Sources/Client/WebSocket/WebSocket.swift

b-onc · 2020-05-13T12:15:06Z

Tests/Virtual Time/VirtualTime.swift

+        while keepRunning {
+
+            let sortedTimers = scheduledTimers
+                .map { (time: $0.nextFireTime(currentTime: now), timer: $0) }


This caused me great confusion.
scheduledTimers is [Timer] but Timer didn't have nextFireTime func. I went back and forth between commits and realized this Timer is actually VirtualTime.Timer
Timer, VirtualTime.Timer and VirtualTimeTimer are all too similar. I suggest we find better names.

b-onc · 2020-05-13T12:19:26Z

Tests/Virtual Time/VirtualTime.swift

+        while keepRunning {
+
+            let sortedTimers = scheduledTimers
+                .map { (time: $0.nextFireTime(currentTime: now), timer: $0) }


Can we rename time to nextFireTime in this tuple? It becomes very confusing reading the code afterwards.

b-onc · 2020-05-13T12:21:49Z

Tests/Virtual Time/VirtualTime.swift

+        self.now = initialTime
+    }
+
+    func run(numberOfSeconds: Seconds? = nil) {


Can you add some documentation to this func/ class? It's hard to grasp what's going on.

b-onc · 2020-05-13T12:24:11Z

Tests/Virtual Time/VirtualTime.swift

+    var timeAdvanceExecutionDelay: TimeInterval = 0.01
+
+    var scheduledTimers: [Timer] = []
+    var now: Seconds


I suggest renaming this to current or currentTime

b-onc · 2020-05-13T12:47:06Z

Tests/Virtual Time/VirtualTimers.swift

+extension VirtualTime.Timer: TimerControl {
+    func resume() {
+        isActive = true
+    }
+
+    func suspend() {
+        isActive = false
+    }
+}
+


IMHO this should be in the implementation and VirtualTimeTests should use resume and suspend, instead of setting directly

I think there's nothing wrong about setting isActive directly in the tests. No need to complicate things. This is not public api.

Why would we use internal API in tests? It's safer to be close to real-life usage scenario, in which we'll never access isActive directly. I also disagree it's complicating, on the contrary I think accessing isActive is more complicated than just timer.suspend() or resume()

Bahadir, we're talking about VirtualTimer which is used solely in tests, never in production. If mutating a variable directly bothers you for some reason, I'll add the functions you want 🤷‍♂️

That's a fair point, but then why do we have the extensions? If we have functions to mutate these variables, we should only use them.
Even if it's testing something only used for testing, API could be consistent.
I accept your point though, it's up to you, we can keep it.

We have it because both simple and repeating timers are represented by the same object in tests, while in the production, we handle them differently. DispatchQueue.asyncAfter for simple timers, and DispatchSourceTimer for repeating timers.

I'll change it so it has the api you want.

b-onc · 2020-05-13T13:01:38Z

Tests/Client/Unit Tests/WebSocket/WebSocketProviderMock.swift

+    /// How many times was `connect()` called
+    var connectCalledCount = 0


Can we group variables and functions? Comment makes it obvious anyway

What do you mean?

ping @b-onc

Putting variables on top, functions after them, and not mixing the order

b-onc · 2020-05-13T13:02:32Z

Tests/Client/Unit Tests/WebSocket/WebSocketProviderMock.swift

+// MARK: - Functions to simulate behavior
+
+extension WebSocketProviderMock {


Can we just use the mark inside main class and not an extension?

b-onc · 2020-05-13T13:09:34Z

Tests/Client/Unit Tests/WebSocket/WebSocketTests.swift

-        reconnectingExpectation.expectedFulfillmentCount = 2
+    func test_reconnectionFlow_withoutStopError() {
+        // Setup (connect)
+        test_connectionFlow()


We shouldn't call other tests within a test case, just connect and simulateconnect would be fine

I'm not super excited about this either. However, I ended up having the almost exact implementation in two functions. If you feel strongly about this, I'll change it. But it's a worse-of-two-evils kind of decision.

OK Let's keep it then 👍

b-onc · 2020-05-13T13:17:05Z

Tests/Client/Unit Tests/WebSocket/WebSocketTests.swift

+    func test_pingIsSentPeriodically() {
+        // Setup (connect)
+        test_connectionFlow()
+        assert(WebSocket.pingTimeInterval == 25)


I don't think this makes sense here. We shouldn't assume ping interval will be 25, and write the test with accessing this value (calculated)

b-onc · 2020-05-15T13:21:33Z

Tests/Virtual Time/VirtualTimers.swift

-        }
-    }
-}
+extension VirtualTime.TimerControl: TimerControl { }


Why don't we put it in VirtualTime.swift file, or directly to implementation?

VirtualTime.swift is a file independent on StreamChatClient. You can take it and include to any target you want. VirtualTimers.swift is where VirtualTime is used to implement internal protocols StreamChatClient.Timer and StreamChatClient.TimerControl. It's good to have them separate.

b-onc · 2020-05-15T13:21:55Z

Tests/Virtual Time/VirtualTime.swift

+
+

Unnecessary whitespace?

b-onc

2 minor comments, other than that 🚢

We will use this to inject types into objects.

This allows us to mock timers in tests and run them much faster.

This makes it possible to run timers in tests faster.

VojtaStavik added 🙏 Feature Request A new feature request sdk:client labels May 13, 2020

VojtaStavik requested review from buh and b-onc May 13, 2020 07:47

VojtaStavik changed the title ~~[CIS 108] Improve WebSocket tests~~ [CIS-108] Improve WebSocket tests May 13, 2020

b-onc reviewed May 13, 2020

View reviewed changes

VojtaStavik requested a review from b-onc May 15, 2020 13:15

b-onc reviewed May 15, 2020

View reviewed changes

Tests/Virtual Time/VirtualTime.swift Outdated

Comment on lines 133 to 134

Copy link

Contributor

b-onc May 15, 2020

Choose a reason for hiding this comment

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

Unnecessary whitespace?

b-onc approved these changes May 15, 2020

View reviewed changes

VojtaStavik added 8 commits May 15, 2020 15:39

Allow variable names starting with capital letter

5c3d161

We will use this to inject types into objects.

Introduce wrappers for timers

b5e1e28

This allows us to mock timers in tests and run them much faster.

Inject Timer type to WebSocket

01a778c

Introduce VirtualTime for tests

a9c60ba

This makes it possible to run timers in tests faster.

Create Timer implementation which uses VirtualTimer

d06987b

Update WebSocket tests to use VirtualTime

6b9a092

Make WebSocket.pingTimeInterval immutable again

508619e

Fix formatting

29e8ae6

VojtaStavik force-pushed the CIS-108-continue branch from 2cd9ded to 29e8ae6 Compare May 15, 2020 13:42

VojtaStavik merged commit a4d07bb into master May 15, 2020

delete-merged-branch bot deleted the CIS-108-continue branch May 15, 2020 13:52

VojtaStavik mentioned this pull request Jun 11, 2020

[CIS-168] Finish WebSocketClient implementation Part 1: Preparation #304

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CIS-108] Improve WebSocket tests #268

[CIS-108] Improve WebSocket tests #268

VojtaStavik commented May 13, 2020

Stream-SDK-Bot commented May 13, 2020

b-onc left a comment

b-onc May 13, 2020

b-onc May 13, 2020

b-onc May 13, 2020

b-onc May 13, 2020

b-onc May 13, 2020

VojtaStavik May 15, 2020

b-onc May 15, 2020

VojtaStavik May 15, 2020

b-onc May 15, 2020

VojtaStavik May 15, 2020

b-onc May 13, 2020

VojtaStavik May 13, 2020

VojtaStavik May 14, 2020

b-onc May 14, 2020

b-onc May 13, 2020

b-onc May 13, 2020

VojtaStavik May 15, 2020 •

edited

Loading

b-onc May 15, 2020

b-onc May 13, 2020

b-onc May 15, 2020

VojtaStavik May 15, 2020 •

edited

Loading

b-onc May 15, 2020

b-onc left a comment

		/// How many times was `connect()` called
		var connectCalledCount = 0

		// MARK: - Functions to simulate behavior

		extension WebSocketProviderMock {

[CIS-108] Improve WebSocket tests #268

[CIS-108] Improve WebSocket tests #268

Conversation

VojtaStavik commented May 13, 2020

In this PR:

Stream-SDK-Bot commented May 13, 2020

b-onc 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

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

Choose a reason for hiding this comment

VojtaStavik May 15, 2020 • edited Loading

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

VojtaStavik May 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-onc left a comment

Choose a reason for hiding this comment

VojtaStavik May 15, 2020 •

edited

Loading

VojtaStavik May 15, 2020 •

edited

Loading