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

Make tests work with both Go 1.13 and 1.14. #4

Closed
wants to merge 1 commit into from

Conversation

ubschmidt2
Copy link
Contributor

Some tests failed with Go 1.14 due to reliance on the exact text of "net/url".Error errors.
Go 1.14 quotes the text of the URL in errors to improve clarity when the URL contains spaces.

For example:

Go 1.13: parse http://example.org: first path segment in URL cannot contain colon
Go 1.14: parse " http://example.org": first path segment in URL cannot contain colon

Some tests failed with Go 1.14 due to reliance on the exact text of "net/url".Error errors.
Go 1.14 quotes the text of the URL in errors to improve clarity when the URL contains spaces.

For example:

  Go 1.13: parse  http://example.org: first path segment in URL cannot contain colon
  Go 1.14: parse " http://example.org": first path segment in URL cannot contain colon
@apparentlymart
Copy link
Contributor

Hi @ubschmidt2! Thanks for working on this.

As part of testing #5 I ended up implementing fixes for these failures in my local tree, which I ended up merging as part of that PR just to reduce the number of steps I was taking.

We typically write our tests targeting a "current" Go version, rather than writing more complex tests to accommodate differences between versions. This particular codebase doesn't have a "current" Go version documented because apparently it doesn't have automatic CI testing set up (!) but I decided to treat 1.14 as the current version here because this codebase primarily exists in support of Terraform and Terraform is currently targeting 1.14 on its main branch too. Therefore what I merged is just updates to the simple error message strings, rather than introducing regular expression matching here.

Thanks again for working on this, and sorry we didn't spot this PR sooner! (I'm afraid I only ended up over here because we were discussing #5 in an issue in the Terraform repository. 😖 )

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.

2 participants