Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve test stability #276

Merged
merged 4 commits into from May 15, 2020
Merged

Improve test stability #276

merged 4 commits into from May 15, 2020

Conversation

VojtaStavik
Copy link
Contributor

@VojtaStavik VojtaStavik commented May 14, 2020

In this PR:

  • MakeRequestRecorderURLProtocol record only selected requests if needed

#no_changelog

SilentMessageTests were accessing shared client implicitly, now they do explicitly. It's not safe to use non-singleton client.
QueryEncodingTests are now deterministic.
Copy link
Contributor Author

@VojtaStavik VojtaStavik left a comment

Choose a reason for hiding this comment

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

Looks good 👍
I wouldn't mind having the change in 3 commits instead of one ;-) :

- Use `sharedClient` in `SilentMessageTests`
- Create `AssertJSONEqual` helper
- Use `AssertJSONEqual` in `QueryEncodingTests `

Comment on lines +24 to +25
func AssertJSONEqual(_ expression1: @autoclosure () throws -> Data,
_ expression2: @autoclosure () throws -> Data,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure about the expressions returning Data but we will probably create a number of convenient overrides for this as we will use it more. All good 👍

Comment on lines +13 to +15
func error(domain: String, code: Int = -1, message: @autoclosure () -> String) -> NSError {
NSError(domain: domain, code: code, userInfo: ["message:": message()])
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd suggest making this private so we don't pollute the module namespace.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also using it in QueryEncodingTests though. Should i move it into its own file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. I think it's OK as is then.

@VojtaStavik VojtaStavik requested a review from buh May 15, 2020 12:03
@VojtaStavik
Copy link
Contributor Author

I need one ✅ to be able to merge this :)

Copy link
Contributor

@b-onc b-onc left a comment

Choose a reason for hiding this comment

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

Me: commits
Also me: ✅

@VojtaStavik
Copy link
Contributor Author

@b-onc
image

@VojtaStavik VojtaStavik merged commit 489c909 into master May 15, 2020
@delete-merged-branch delete-merged-branch bot deleted the device-tests-2 branch May 15, 2020 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants