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

accounts/url: add test logic what check null string to parseURL() #25013

Closed
wants to merge 4 commits into from
Closed

Conversation

dbadoy
Copy link
Contributor

@dbadoy dbadoy commented Jun 2, 2022

accounts/url: add test logic what check null string to parseURL() #25011

Comment on lines 40 to 43
_, err = parseURL("")
if err == nil {
t.Error("expected err, got: nil")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

An alternative way to do this which is a bit more easily extendable would be this:

	for _, u := range []string{"ethereum.org", ""}{
		if _, err = parseURL(u); err == nil {
			t.Errorf("input %v, expected err, got: nil", u)
		}
	}

As it's a kind of small test though, I don't have a problem with the way you've done it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

appreciate it.
It's applied.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if len(parts) != 2 || parts[0] == ""
// parts[0] == "" is this necessary?

parts := strings.split(url, "://")
// "ethereum.org"
// ""
// length: 1

Copy link

@tbcd tbcd Jun 4, 2022

Choose a reason for hiding this comment

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

That's because parseURL("") does not test anything more than parseURL("ethereum.org") already tested.

It does not test the || parts[0] == "" branch that you intended. It would need to be something like parseURL("://ethereum.org").

Edit: Re-reading the issue title and PR title, it seems like you misunderstood the purpose of the branch in the first place. It was to test for an empty scheme string result, not an empty URL string being passed into the function.

@cesarvspr
Copy link

File is not `goimports`-ed

This error is being raised, any idea why? @dbadoy

@dbadoy
Copy link
Contributor Author

dbadoy commented Jun 6, 2022

Sorry. close this PR because there was a mistake in the merge.

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.

5 participants