[Chore] Validate uniqueness of UI test keys

[CodeBaseCamp]

Why
Executing the uiTest methods with duplicate keys causes previously created images to be overridden. This is usually undesirable and should be prevented programmatically.

Changes
A singleton UITestCaseKeyValidator instance for validating the uniqueness of UI test keys is introduced and used in the relevant methods.

[danieleformichelli]

Shouldn't we add some locking?
Not sure if tests are executed in parallel right now, but it might be the case in the future

[CodeBaseCamp]

Good call, done.

[danieleformichelli]

Here we should print the intersection between self.keys and keys, not the whole keys

[CodeBaseCamp]

Done.

[FEC-bendingspoons]

I left a couple of improvements, so that the usage of this is a bit more ergonomic. I have however some concerns:

• are we sure that this is the best way to solve this problem? I feel that detecting these at runtime is too late (i.e. waiting for the uitest to finish before finding out). I feel that implementing a simple script that does a similar job in the linting phase is much more useful, as it wouldn't even let the project build
• this is not thread safe. this is super bad, since test might (and will!) run in parallell
• are your sure that a singleton is the best approach here? probably yes (since we are not the ones launching the tests) but it might be worth to investigate

[FEC-bendingspoons]

Suggested change

	private var keys = Set<String>()
	/// The keys are the uitest keys, the values are the test case that implements them
	private var knownKeys: [String: String] = [:]

[CodeBaseCamp]

Done.

[FEC-bendingspoons]

Suggested change

	func validate(keys: Set<String>) {
	func validate(keys: Set<String>, testCaseName: String) {

[CodeBaseCamp]

Done.

[FEC-bendingspoons]

Suggested change

	guard self.keys.isDisjoint(with: keys) else {
	fatalError("Key duplication: \(keys)")
	}
	keys.forEach {
	self.keys.insert($0)
	}
	keys.forEach {
	if let kownTestCaseName = self.knownKeys[$0] {
	fatalError("Duplication detected. Key \($0) used both in \(testCaseName) and \(knownTestName)")
	}
	self.knownKeys[$0] = testCaseName
	}

[CodeBaseCamp]

Done.

[FEC-bendingspoons]

Suggested change

	UITestCaseKeyValidator.singletonInstance.validate(keys: Set(testCases.keys))
	UITestCaseKeyValidator.singletonInstance.validate(keys: Set(testCases.keys), testCaseName: "\(Self.self)")

[CodeBaseCamp]

Done.

[FEC-bendingspoons]

Suggested change

	UITestCaseKeyValidator.singletonInstance.validate(keys: Set(testCases.keys))
	UITestCaseKeyValidator.singletonInstance.validate(keys: Set(testCases.keys), testCaseName: "\(Self.self)")

[CodeBaseCamp]

Done.

[CodeBaseCamp]

@FEC-bendingspoons Great comments, thanks!

are we sure that this is the best way to solve this problem? I feel that detecting these at runtime is too late (i.e. waiting for the uitest to finish before finding out). I feel that implementing a simple script that does a similar job in the linting phase is much more useful, as it wouldn't even let the project build

• I initially thought about a script as well but we might miss cases (unless we make the script complicated) because the array of keys with which the uiTest methods are invoked may be defined before the actual invocation, so I think that the current solution is a good balance of the guarantee we want to achieve and the speed with which we find potential issues.

this is not thread safe. this is super bad, since test might (and will!) run in parallell

• Totally agree, I fixed this.

are your sure that a singleton is the best approach here? probably yes (since we are not the ones launching the tests) but it might be worth to investigate

• Using a singleton makes it easier to avoid mistakes arising from inserting keys to different instances, possibly missing a duplication. Do you have a better idea in mind?

[CodeBaseCamp]

Also, @danyf90 thanks for pointing out the lacking thread-safety, I should have noticed this myself.