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

fix for flaky renameuser test using polling-until-expected-result #493

Conversation

NortySpock
Copy link
Contributor

@NortySpock NortySpock commented Sep 28, 2024

Changes

Potential risks

... I'm actually not sure how long it polls or what the timeout is -- if it never times out the poll would run forever, and thus the test would never finish.

I'm open to critiques or improvements on this PR. I am open to troubleshooting and trying to fix other tests after this if this PR is accepted.

@NortySpock NortySpock marked this pull request as draft September 28, 2024 19:59
@NortySpock
Copy link
Contributor Author

Looks like the error message from the test is

Error:      test/teiserver/protocols/spring/spring_auth_test.exs:459
     Assertion with == failed
     code:  assert reply == "SERVERMSG Invalid characters in name (only a-z, A-Z, 0-9, [, ] allowed)\n"
     left:  "SERVERMSG Invalid characters in name (only a-z, A-Z, 0-9, [, ] allowed)\nSAIDPRIVATE Coordinator Invalid characters in name (only a-z, A-Z, 0-9, [, ] allowed)\n"
     right: "SERVERMSG Invalid characters in name (only a-z, A-Z, 0-9, [, ] allowed)\n"
     stacktrace:
       test/teiserver/protocols/spring/spring_auth_test.exs:475: (test)

This test failure is not occurring when I run locally, but on the other hand I am getting other errors locally that indicate some sort of admin permissions problem, so possibly my local setup is incomplete.

 69) test update user renders errors when data is invalid (TeiserverWeb.Admin.UserControllerTest)
     test/teiserver_web/controllers/admin/user_controller_test.exs:94
     ** (KeyError) key :management_roles not found in: %{
       user: %Teiserver.Account.User{
         __meta__: #Ecto.Schema.Metadata<:loaded, "account_users">,
         id: 1810,
         name: "Current user",

…sult

relax RENAMEACCOUNT test since apparently more information is returned from the server now

formatted code
@NortySpock NortySpock force-pushed the fix-flaky-renamuser-test-with-polling branch from 0d026bb to 2f4f253 Compare September 28, 2024 21:09
@NortySpock
Copy link
Contributor Author

NortySpock commented Oct 3, 2024

Somehow managing to get (in my local build) in the PR branch than in master, even when I pin the seed and exclude ignored tests...

Since it does not seem I'm making progress on this front, I will likely close this PR and attempt to work on a different issue.

Nevermind, I had forgotten to restart the server I was running that the unit-tests were bouncing off of.

I'm a little surprised this worked when I had several other tests fail locally, but, in the end, this re-enables a test and makes it not nearly as flaky.

@NortySpock NortySpock marked this pull request as ready for review October 3, 2024 02:28
@NortySpock NortySpock changed the title partial fix for flaky renameuser test using polling-until-expected-result fix for flaky renameuser test using polling-until-expected-result Oct 3, 2024

# Rename with existng name
_send_raw(socket, "RENAMEACCOUNT #{watcher_user.name}\n")
reply = _recv_raw(socket)
assert reply == "SERVERMSG Username already taken\n"
assert String.starts_with?(reply, "SERVERMSG")
assert String.contains?(reply, "Username already taken")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine as it is. But you can leverage Elixir's pattern matching for something a little bit more direct:

assert "SERVERMSG" <> rest = _recv_raw(socket)
assert String.contains?(rest,)

test/teiserver/protocols/spring/spring_auth_test.exs Outdated Show resolved Hide resolved
test/teiserver/protocols/spring/spring_auth_test.exs Outdated Show resolved Hide resolved
@L-e-x-o-n L-e-x-o-n merged commit 94fbe0e into beyond-all-reason:master Oct 20, 2024
2 of 3 checks passed
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