-
Notifications
You must be signed in to change notification settings - Fork 23
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
[BUG] Fix End-to-End test suite #189
Conversation
"github.com/stretchr/testify/require" | ||
) | ||
|
||
func TestExperimentRunner_GetTreatmentForRequest(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this test as an initial check to see if the sample experiment's engine works (as expected) :)
Thanks a lot for coming up with this!
I went to take a closer look at how the example experiment engine and the batch prediction engine works but nothing seems to suggest non-deterministic behaviour as you suggested. Maybe it's as you have mentioned, just due to the original way the responses were compared, which is interesting how it throws out results that were seemingly random. Once again, thanks a lot for the fix! |
@@ -81,40 +81,62 @@ func TestCreateRouter(t *testing.T) { | |||
assert.Equal(t, http.StatusOK, response.StatusCode, | |||
"Unexpected response (code %d): %s", | |||
response.StatusCode, string(responsePayload)) | |||
actualResponse := gjson.GetBytes(responsePayload, "#.data.response").String() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somehow when you rewrote the entire expected and actual responses explicitly, the tests seem to work perfectly fine but I'm still unsure of the original cause of the non-deterministic test outcomes initially. One possible (but still unlikely I feel) cause might've been the way the responses were extracted from the original JSON body with the gjson
package in that the response order might not have been preserved, but I can't confirm this 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, may assumptions are along these lines too
Context:
#184 contained the updates to the e2e tests to cover the deployment of a Turing Router with the experiment engine plugin. It turned out, that one of the tests started failing intermittently because of the non-deterministic behavior (#188 (comment)).
The problem occurs only with the test, that asserts the
/batch_predict
endpoint response. Previously, the request payload to the/batch_predict
contained two equal prediction request payloads (the entire request was[{}, {}]
), however, #184 has changed this to verify that the experiment engine assigns treatments correctly to different requests.I'm not able to verify, that the experiment engine assigns treatments non-deterministically, so I assume that the problem was observed because of how the expected and actual responses were compared.
This PR adds a unit test for the test experiment engine's
ExperimentRunner. GetTreatmentForRequest
implementation and updates the e2e test part of the/batch_predict
response assertion.