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

[BUG] Fix End-to-End test suite #189

Merged
merged 6 commits into from
Mar 28, 2022

Conversation

romanwozniak
Copy link
Contributor

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.

@romanwozniak romanwozniak self-assigned this Mar 25, 2022
@romanwozniak romanwozniak requested a review from a team March 25, 2022 11:02
@romanwozniak romanwozniak added the type: bug Something isn't working label Mar 25, 2022
"github.com/stretchr/testify/require"
)

func TestExperimentRunner_GetTreatmentForRequest(t *testing.T) {
Copy link
Contributor

@deadlycoconuts deadlycoconuts Mar 28, 2022

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) :)

@deadlycoconuts
Copy link
Contributor

Thanks a lot for coming up with this!

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.

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()
Copy link
Contributor

@deadlycoconuts deadlycoconuts Mar 28, 2022

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 👀

Copy link
Contributor Author

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

@romanwozniak romanwozniak merged commit 6d9df86 into caraml-dev:main Mar 28, 2022
@romanwozniak romanwozniak deleted the fix_e2e_tests branch March 28, 2022 05:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants