Use HTTPS in test.com test URL
#8338
Conversation
I noticed a runtime warning in the Wormholy library: > App Transport Security has blocked a cleartext HTTP connection > totest.comsince [sic] it is insecure. Use HTTPS instead or add this domain > to Exception Domains in your Info.plist.
mokagio
force-pushed
the
mokagio/use-https-for-test-site
branch
from
ce85caf to
56cb936
Compare
| @@ -6,6 +6,9 @@ import Yosemite | |||
|
|
|||
| /// Test cases for `AuthenticationManager`. | |||
| final class AuthenticationManagerTests: XCTestCase { | |||
|
|
|||
| let testSiteURL = "https://test.com" | |||
There was a problem hiding this comment.
DRYed the value while at it.
DRYing literals in unit tests can sometimes backfire because it reduces readability, or rather how expressive the tests are.
In this case, though, the URL is not used in any assertion so there's no loss of clarity.
You can test the changes from this Pull Request by:
|
I didn't drill into them, but they are all in the diff. Meaning the code that I changed were the tests cases hitting
I'm guessing no, too. This might be something we should research further, e.g. by running the tests with the network off. Or, given the warning originated from Wormholy, which is a network debugging library, maybe that's expected? Although, I think the two things might be unrelated—Wormholy was hit, but that, too, was an oversight in a test that should have been configured to bypass the network altogether. cc @pmusolino as Wormholy's author and WooCommerce contributor. |
| func test_it_can_display_jetpack_error_for_org_site_credentials_sign_in() { | ||
| // Given | ||
| let manager = AuthenticationManager() | ||
| let testSite = "http://test.com" | ||
| let siteInfo = WordPressComSiteInfo(remote: ["isWordPress": true, "hasJetpack": false, "urlAfterRedirects": testSite]) | ||
| let siteInfo = WordPressComSiteInfo(remote: ["isWordPress": true, "hasJetpack": false, "urlAfterRedirects": testSiteURL]) |
There was a problem hiding this comment.
This test failed 2 out of 2 times in CI 🤔
Maybe there's more work I need to do. Reverting to draft.
Ooh, I might've misread the code. I thought not all of them actually send HTTP request to the site URL, like some may just need a URL string to create a type or call a function. I was more curious if your observation here exposed a potentially overlooked test set up, like missing a stub for this test.com HTTP request. However, IMO, even if it's an overlook, it's probably okay not to stub the request as it's not affecting the test case obviously. |
Description
I got a runtime warning when running the unit tests:
Testing instructions
If CI is green we're good. This can be further verified by running the unit tests locally and verifying there's no warning about
test.comRELEASE-NOTES.txtif necessary.