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

create new TestDaemons for each test cases #177

Merged
merged 1 commit into from
Jan 27, 2020

Conversation

shogo82148
Copy link
Contributor

Issue #, if available:

I'm guessing this pull request fixes some broken tests. e.g. https://travis-ci.org/aws/aws-xray-sdk-go/jobs/637161870

Description of changes:

By default, go test run tests in parallel.
So multiple TestDaemons try to listen UDP on 127.0.0.1:2000 in almost same time,
and conflict each other.

https://golang.org/cmd/go/#hdr-Compile_packages_and_dependencies

Compile packages and dependencies

-p n
  the number of programs, such as build commands or
  test binaries, that can be run in parallel.
  The default is the number of CPUs available.

https://golang.org/cmd/go/#hdr-Testing_flags

Testing flags

-parallel n
    Allow parallel execution of test functions that call t.Parallel.
    The value of this flag is the maximum number of tests to run
    simultaneously; by default, it is set to the value of GOMAXPROCS.
    Note that -parallel only applies within a single test binary.
    The 'go test' command may run tests for different packages
    in parallel as well, according to the setting of the -p flag
    (see 'go help build').

I created new TestDaemons for each test cases to avoid this.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@shogo82148 shogo82148 force-pushed the fix-daemon-for-testing branch from af26220 to dc2ab1b Compare January 16, 2020 09:42
@bhautikpip
Copy link
Contributor

bhautikpip commented Jan 16, 2020

Hi @shogo82148,

I have investigated test case failures issue and it looks like we are not resetting a daemon because of that daemon sends incorrect data for some tests and this inconsistency happens after 10-15 retries. So I will create a PR which adds TestDaemon.Reset() before calling the TestDaemon.Recv() method. It solves the inconsistency of tests. However I am not sure why we are seeing error logs of connection refused. I saw your description of this PR and looks like could be the reason when multiple test daemon try to listen simultaneously. I saw the error of this PR and it still shows the error logs. Do you have any idea how can we get rid of those warnings of connection refused ?

@shogo82148
Copy link
Contributor Author

NewTestDaemon overrides the daemon's address, but some clients to connect the default address 127.0.0.1:2000.

https://github.com/aws/aws-xray-sdk-go/pull/177/checks?check_run_id=392856538#step:6:93

2020-01-16T09:50:59Z [ERROR] write udp 127.0.0.1:33701->127.0.0.1:2000: write: connection refused

It means someone else changed the configure...
maybe the environment values?

@bhautikpip
Copy link
Contributor

I am not sure but tests are behaving inconsistent.

@bhautikpip
Copy link
Contributor

bhautikpip commented Jan 17, 2020

I have merged one PR today to fix resetting the daemon issue but looks like there is one more issue around TestDaemon.Recv() which fails the tests. (https://github.com/aws/aws-xray-sdk-go/runs/394354285)

@bhautikpip
Copy link
Contributor

I am also getting the same error that you are getting in this tests. (https://github.com/aws/aws-xray-sdk-go/pull/177/checks?check_run_id=394369996#step:6:80).

@shogo82148
Copy link
Contributor Author

https://github.com/aws/aws-xray-sdk-go/pull/177/checks?check_run_id=394461889#step:6:100

race condition?

Error: Received unexpected error parse //��j�EɳS<�[�y�%2F@�E4�v��A�y�R��s: net/url: invalid userinfo

I'm working on SQL in #169
Just comment out here.

@shogo82148
Copy link
Contributor Author

@shogo82148
Copy link
Contributor Author

Could you review and/or merge, please?

There are still some TODOs, but they are not in the scope of this pull request.
I'm going to work on other pull requests.

@bhautikpip
Copy link
Contributor

Hi @shogo82148,

Thanks for the changes. Just wanted to make sure that these changes solves the inconsistency of tests. would you mind doing an empty commit so that tests run again because I wanted to see if tests are consistent with these changes.

@shogo82148
Copy link
Contributor Author

no, i don't mind.
thank you.

xray/sql_test.go Outdated
s.Require().NoError(s.mock.ExpectationsWereMet())
//s.Equal("gopkg.in/DATA-DOG/go-sqlmock.v1", s.db.driverVersion)
}
// FIXME: @shogo82148
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind elaborating what type of fix this tests require ?

@bhautikpip
Copy link
Contributor

bhautikpip commented Jan 17, 2020

Hi @shogo82148,

I have reviewed this PR. Most of the changes looks good. I have asked most questions related to commenting out few test cases just so we are on the same page and would you mind doing a few empty commit to trigger git tests and Travis CI build ? I am not sure if we can manually start running git tests. I just wanted to double check consistency of this changes.

@shogo82148 shogo82148 requested a review from bhautikpip January 18, 2020 00:42
@bhautikpip
Copy link
Contributor

bhautikpip commented Jan 18, 2020

Looks like something is happening at this line (https://github.com/aws/aws-xray-sdk-go/pull/177/files#diff-05d404a7fd0c9af128e5597bba25efc1R423). There is one test case failed in every commit because of this. Tests look still inconsistent.

@shogo82148
Copy link
Contributor Author

yeah... I'm trying to fix it, but it still fails...
Can I comment out and mark as TODOs?

@bhautikpip
Copy link
Contributor

I was testing this changes made by you to find out why tests are failing. I think if you comment out func TestRoundTripReuseDatarace(t *testing.T), func TestRoundTripReuseTLSDatarace(t *testing.T) and func TestRoundTripHttp2Datarace(t *testing.T) in client_test.go then tests will pass consistently and I have tested that. So my guess would be data race is happening inside these functions might want to look at that. I am not sure about what you have asked might want to consult with my team and get back to you if that works.

@shogo82148
Copy link
Contributor Author

I commented out these tests.

Currently some tests fail, but due to this pull request.
I issued that.
https://github.com/aws/aws-xray-sdk-go/pull/177/checks?check_run_id=397261225#step:6:55
#180

@bhautikpip
Copy link
Contributor

I ran tests again. I think that error is related to the issue (#180) you have opened and it's very rare still need to investigate on that but you will not see (https://github.com/aws/aws-xray-sdk-go/runs/396293215#step:6:75) this error after commenting out those functions in client_test.

@shogo82148 shogo82148 force-pushed the fix-daemon-for-testing branch 2 times, most recently from 3399e00 to f1811db Compare January 19, 2020 06:05
@shogo82148
Copy link
Contributor Author

I misunderstood -parallel n option...
parallel execution is enabled only if we call t. Parallel
so touching the environment is safe.

@shogo82148 shogo82148 force-pushed the fix-daemon-for-testing branch 4 times, most recently from b95727b to d998ad3 Compare January 20, 2020 00:36
@bhautikpip
Copy link
Contributor

Hi @shogo82148,

Do you think test cases are consistent now with this changes ?

@shogo82148
Copy link
Contributor Author

Yes, the are.

unfortunately, I give up fixing TestRoundTripReuseDatarace, TestRoundTripReuseTLSDatarace and TestRoundTripReuseHTTP2Datarace completely... 🙁
The aim of these test is detecting data race of http clients,
so there is no need to check that ALL X-Ray segments are received by the X-Ray daemon.
https://github.com/aws/aws-xray-sdk-go/pull/177/files#diff-05d404a7fd0c9af128e5597bba25efc1R340

@bhautikpip
Copy link
Contributor

Hi @shogo82148,

I think you are right. We might not want to check whether all the segments are received by the daemon. Do you think that is the reason causing test case failures ?

@shogo82148
Copy link
Contributor Author

yes.

@bhautikpip
Copy link
Contributor

bhautikpip commented Jan 22, 2020

Hi @shogo82148,

Are you going to work on SQL tests ? I have tested current changes in your branch couple of times. Looks like it passed test cases. I think currently we have 3 issue.

  1. sql_test.go
  2. client_test.go (TestRoundTripReuseDatara, TestRoundTripReuseTLSDatarace, TestRoundTripHttp2Datarace in td.Recv() function. We might drop this code as you have suggested there's no point in checking whether we have received segments or not)
  3. reservoir_test.go this line (https://github.com/aws/aws-xray-sdk-go/blob/master/strategy/sampling/reservoir_test.go#L46) - fails rarely

Should I consider merging this PR ?

@shogo82148
Copy link
Contributor Author

Are you going to work on SQL tests ?

Yes, I am. But now, I'm waiting for the answer of #169

Should I consider merging this PR ?

Yes, please.

@bhautikpip
Copy link
Contributor

Okay. I will expedite the review process for (#169). It looks like a good support. Also, would you mind commenting out this line in reservoir_test.go (https://github.com/aws/aws-xray-sdk-go/blob/master/strategy/sampling/reservoir_test.go#L46) and add a line FIXME or something and remove go.mod changes from the commit (https://github.com/aws/aws-xray-sdk-go/pull/177/files#diff-37aff102a57d3d7b797f152915a6dc16R3) ?

@bhautikpip
Copy link
Contributor

@shogo82148
Copy link
Contributor Author

done!

Copy link
Contributor

@willarmiros willarmiros left a comment

Choose a reason for hiding this comment

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

Thank you very much for your contributions @shogo82148! A few minor comments and this should be good to go.

xray/sql_test.go Outdated
}
// FIXME: @shogo82148 remove race condition.
// see https://github.com/aws/aws-xray-sdk-go/pull/177#issuecomment-575438315
// func TestSQL(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that some of these SQL tests are failing due to data race conditions, however we cannot merge a PR that has an entire file commented out. Even if it causes some checks to fail, as long as we can verify that the only test failures are these SQL tests then the failures are not caused by this PR and it can be merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

@shogo82148 shogo82148 force-pushed the fix-daemon-for-testing branch from 50951f3 to 323b88a Compare January 25, 2020 14:05
Copy link
Contributor

@willarmiros willarmiros left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, looks good!

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.

3 participants