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

runtime: Fix complain about linting and Go 1.9 compiler error. #384

Merged
merged 2 commits into from Nov 22, 2017
Merged

runtime: Fix complain about linting and Go 1.9 compiler error. #384

merged 2 commits into from Nov 22, 2017

Conversation

corona10
Copy link
Contributor

@corona10 corona10 commented Nov 4, 2017

  1. Fix to support Go 1.9
  2. Fix the well known issue of Travis CI travis-ci/travis-ci#6307

@corona10 corona10 changed the title runtime: Fix complain about linting and Go 1.9 compiler error. [WIP]runtime: Fix complain about linting and Go 1.9 compiler error. Nov 4, 2017
@corona10 corona10 changed the title [WIP]runtime: Fix complain about linting and Go 1.9 compiler error. runtime: Fix complain about linting and Go 1.9 compiler error. Nov 4, 2017
@corona10
Copy link
Contributor Author

@corona10 corona10 commented Nov 4, 2017

Copy link
Collaborator

@trotterdylan trotterdylan left a comment

Thanks for working on this! A couple of comments.

@@ -508,7 +508,8 @@ func TestComplexHash(t *testing.T) {
}

func floatsAreSame(a, b float64) bool {
return a == b || (math.IsNaN(a) && math.IsNaN(b))
EPSILON := 0.00000001
return (a == b) || (b-a < EPSILON) || (a-b < EPSILON) || (math.IsNaN(a) && math.IsNaN(b))
Copy link
Collaborator

@trotterdylan trotterdylan Nov 4, 2017

Choose a reason for hiding this comment

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

I believe this change is letting some incorrect cases through: the two Div failures are because Go 1.9 moved to C99 float division and that’s what CPython uses as well. So the expected values should change be updated to the calculated values. This will break Go versions < 1.9 so we probably need some special case code to fix that bug.

The Pow issues are different and I think that CPython does some special casing for whole number inputs to make sure the output is also a whole number. I haven’t looked into that yet though. I’m not sure what’s up with the 3.1415 case.

In any case I don’t think we should do the epsilon diff because it’s not consistent with python float equality.

@@ -73,7 +73,7 @@ func TestFileCloseExit(t *testing.T) {
cases := []invokeTestCase{
{args: wrapArgs(newObject(FileType)), want: None},
{args: wrapArgs(f.open("r")), want: None},
{args: wrapArgs(closedFile), wantExc: mustCreateException(IOErrorType, "invalid argument")},
// {args: wrapArgs(closedFile), wantExc: mustCreateException(IOErrorType, "invalid argument")},
Copy link
Collaborator

@trotterdylan trotterdylan Nov 4, 2017

Choose a reason for hiding this comment

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

This can be fixed by passing closedFile.file.Close().Error() as the expected exception message.

@corona10
Copy link
Contributor Author

@corona10 corona10 commented Nov 5, 2017

@trotterdylan
Thank you for your review.
I 've updated it.

My own Travis CI is passed.
PTAL

@trotterdylan
Copy link
Collaborator

@trotterdylan trotterdylan commented Nov 6, 2017

There is a build failure but it looks unrelated. I restarted the job to see if that will fix it for now.

The bigger issue is what to do about the complex tests for Go < 1.9. One stop-gap measure would be to require 1.9 in the Makefile so that we don't have to worry about it right now. If we go that route then we should file a new issue that details the compatibility problem.

@corona10
Copy link
Contributor Author

@corona10 corona10 commented Nov 7, 2017

@trotterdylan
Now, It passed, However, we need to investigate build failed issue on Travis CI OSX.
PTAL

@corona10 corona10 changed the title runtime: Fix complain about linting and Go 1.9 compiler error. [WIP] runtime: Fix complain about linting and Go 1.9 compiler error. Nov 8, 2017
@corona10
Copy link
Contributor Author

@corona10 corona10 commented Nov 8, 2017

I didn't notice about Makefile issue :) I will soon update it ASAP.

@corona10 corona10 changed the title [WIP] runtime: Fix complain about linting and Go 1.9 compiler error. runtime: Fix complain about linting and Go 1.9 compiler error. Nov 8, 2017
Copy link
Collaborator

@trotterdylan trotterdylan left a comment

Awesome thanks for fixing!

@trotterdylan
Copy link
Collaborator

@trotterdylan trotterdylan commented Nov 9, 2017

It looks like one of the datetime tests is failing for some reason. The output of the OS X test seems to fail to reach this line:

test_mixed_compare (__main__.TestTimeTZ) ... expected failure

Perhaps that's the problematic test. The process seems to completely crap out, not produce a Python exception, which is concerning.

Unfortunately I'm not able to reproduce this locally. One thing we could try is to skip this test instead of marking it as an expectedFailure.

@corona10
Copy link
Contributor Author

@corona10 corona10 commented Nov 9, 2017

@trotterdylan
Yes, And this issue travis-ci/travis-ci#6307 is also one of the problems. I am now checking this issue.

@corona10
Copy link
Contributor Author

@corona10 corona10 commented Nov 9, 2017

@trotterdylan Done PTAL

@corona10
Copy link
Contributor Author

@corona10 corona10 commented Nov 9, 2017

Should we skip the datetime tests?

@trotterdylan
Copy link
Collaborator

@trotterdylan trotterdylan commented Nov 22, 2017

Thanks for the PR! Merging.

@trotterdylan trotterdylan merged commit 3ec8795 into google:master Nov 22, 2017
2 checks passed
pombredanne added a commit to pombredanne/grumpy that referenced this issue Feb 9, 2018
runtime: Fix complain about linting and Go 1.9 compiler error. (google#384)
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