feat: Add function to perform domain checks #22
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wedamija
commented
Jun 4, 2024
wedamija
commented
Jun 4, 2024
src/checker.rs
Outdated
| } | ||
| } | ||
|
|
||
| return CheckResult::FAILURE(FailureReason::Error(format!("{:?}", e))); |
There was a problem hiding this comment.
Probably failure should include the status code too? (If present)
src/checker.rs
Outdated
Comment on lines
72
to
113
| mod tests { | ||
| use super::{check_domain, CheckResult, FailureReason}; | ||
|
|
||
| #[tokio::test] | ||
| async fn test_add() { | ||
| assert_eq!( | ||
| check_domain("https://google.com".to_string()).await, | ||
| CheckResult::SUCCESS | ||
| ); | ||
| assert_eq!( | ||
| check_domain("https://sentry.io".to_string()).await, | ||
| CheckResult::SUCCESS | ||
| ); | ||
| assert_eq!(check_domain("https://hjkhjkljkh.io".to_string()).await, CheckResult::FAILURE(FailureReason::DnsError("failed to lookup address information: nodename nor servname provided, or not known".to_string()))); | ||
| assert_eq!(check_domain("https://santry.io".to_string()).await, CheckResult::FAILURE(FailureReason::DnsError("failed to lookup address information: nodename nor servname provided, or not known".to_string()))); | ||
| } | ||
| } |
There was a problem hiding this comment.
This requires internet to run right? ☠️
There was a problem hiding this comment.
Yeah, I'm going to put mocks here later
vartec
reviewed
Jun 4, 2024
src/checker.rs
Outdated
| Error(String), | ||
| } | ||
|
|
||
| pub async fn check_domain(domain: String) -> CheckResult { |
There was a problem hiding this comment.
You're using URLs without trailing /, so you get 301 redirects, which Reqwest handles automatically, but it's kinda unnecessary.
loewenheim
previously requested changes
Jun 4, 2024
wedamija
commented
Jun 4, 2024
wedamija
commented
Jun 5, 2024
| @@ -8,6 +8,8 @@ edition = "2021" | |||
| [dependencies] | |||
| anyhow = "1.0.66" | |||
| clap = { version = "4.4.6" } | |||
| httpmock = "0.7.0-rc.1" | |||
There was a problem hiding this comment.
I chose this over mockito since it also has the ability to simulate a delay/timeout
loewenheim
reviewed
Jun 5, 2024
Add initial function to perform domain checks. We need better testing around this and probably also need return more useful failure reasons. I'll follow up later with a pr that adds mocking for the domains so we can test all the cases.
wedamija
force-pushed
the
danf/check-domain
branch
from
ae053d3
to
68c0d3b
Compare
wedamija
dismissed
loewenheim’s stale review
Feedback is addressed, we'll follow up with clippy afterwards
evanpurkhiser
approved these changes
Jun 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add initial function to perform domain checks. We need better testing around this and probably also need return more useful failure reasons.
I'll follow up later with a pr that adds mocking for the domains so we can test all the cases.