HackIllinois / api

The Official API supporting HackIllinois
https://api.hackillinois.org
Other
23 stars 51 forks source link

Race condition within `add_accept_finalize_user_test` #508

Closed Nydauron closed 1 year ago

Nydauron commented 1 year ago

When running add_accept_finalize_user_test, it can be observed that it indeed seems to be a race problem, but I honestly can't seem to pinpoint the issue. I tried adding:

func TestAddApproveFinalizeUsers(t *testing.T) {
    defer client.Database(user_db_name).Drop(context.Background())
    defer client.Database(decision_db_name).Drop(context.Background())

to add cleanup after test completion, but it still exists.

I then change the sling response structs to be:

api_err := errors.ApiError{}
response, err := admin_client.New().Post("decision/").BodyJSON(decision_request).Receive(&received_decision_history, &api_err)
if err != nil {
    t.Errorf("Unable to make request")
}
if response.StatusCode != 200 {
    t.Errorf("Status: %v, Response: %v", response.Status, api_err)
}

(i.e. added an errors.ApiError struct and gave that as the struct if the request fails) to see the specific error that was occurring. I then ran the test with -count 10 and got some of the following outputs:

Failed on POST /decision/:

add_accept_finalize_user_test.go:117: Status: 500 Internal Server Error, Response: {500 INTERNAL_ERROR Could not update decision. Error: UNKNOWN}
add_accept_finalize_user_test.go:122: Failed to accept user. Got {false   0  0 0 []}

Failed on POST /decision/:

add_accept_finalize_user_test.go:117: Status: 500 Internal Server Error, Response: {500 DATABASE_ERROR Could not fetch updated decision. Error: NOT_FOUND}
add_accept_finalize_user_test.go:122: Failed to accept user. Got {false   0  0 0 []}

Failed on POST /decision/finalize/:

add_accept_finalize_user_test.go:140: Status: 500 Internal Server Error, Response: {500 DATABASE_ERROR Could not fetch updated decision. Error: NOT_FOUND}
add_accept_finalize_user_test.go:145: Failed to finalize user. Got {false   0  0 0 []}

Typically, if POST /decision/ failed, POST /decision/finalize/ also failed.

The second and third errors confuse me a lot because that error should (in theory, under ideal conditions) should never send the phrase "Could not fetch updated decision" since it is getting a document that it literally just upserted.

It honestly might be a case of the actual endpoint being poorly designed (e.g. we swap out some of the db instructions for more Mongo atomic instructions like FindOneAndReplace()).

Nydauron commented 1 year ago

Also, add_accept_finalize_user_test always seems to pass the first time, but it is not guaranteed to pass any subsequent runs later.