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.

Sign up for GitHub

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

Jump to bottom

Write tests for account #12

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Write tests for account #12

wants to merge 2 commits into from

Conversation

slsyy
Copy link
Contributor

@slsyy slsyy commented May 11, 2024 •
edited
Loading

please tell me, if you like the approach

tests need a little bit more cleaning and polishing, but i post it early to gather a feedback

i can also test other routes in the future

UpcraftLP
UpcraftLP requested changes May 11, 2024
View reviewed changes
Copy link
Collaborator

@UpcraftLP UpcraftLP left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looking great here, however, instead of validating the currrent (wrong) behavior, please change the tests to validate the desired behavior as indicated, so that the tests fail and can be fixed.
Note: Do not fix them in this PR please, as that may require those changes to be mirrored to the frontend.

rogueserver_test.go Outdated Show resolved Hide resolved
rogueserver_test.go Outdated Show resolved Hide resolved
rogueserver_test.go Outdated Show resolved Hide resolved
rogueserver_test.go Outdated Show resolved Hide resolved
rogueserver_test.go Outdated Show resolved Hide resolved
rogueserver_test.go Outdated Show resolved Hide resolved
rogueserver_test.go Outdated Show resolved Hide resolved
rogueserver_test.go Outdated Show resolved Hide resolved
rogueserver_test.go Outdated Show resolved Hide resolved
rogueserver_test.go Outdated Show resolved Hide resolved
@slsyy
Copy link
Contributor Author

slsyy commented May 11, 2024 •
edited by UpcraftLP
Loading

Looking great here, however, instead of validating the current (wrong) behavior, please change the tests to validate the desired behavior as indicated, so that the tests fail and can be fixed. Note: Do not fix them in this PR please, as that may require those changes to be mirrored to the frontend.

My approach was to not change anything due to the those changes to be mirrored to the frontend except obvious bugs like a fix in a change password handler, which does not work right now

So next steps: I will fix the API first with backend <-> frontend manual tests, then I want to merge that one with correct behavior already on master

@UpcraftLP
Copy link
Collaborator

My approach was to not change anything due to the those changes to be mirrored to the frontend except obvious bugs like a fix in a change password handler, which does not work right now

So next steps: I will fix the API first with backend <-> frontend manual tests, then I want to merge that one with correct behavior already on master

Having failing tests is fine, I would merge the PR regardless.

@slsyy
Copy link
Contributor Author

slsyy commented May 11, 2024

Having failing tests is fine, I would merge the PR regardless.

If it is not blocking anything (like docker build and push, i don't know) then why not

@slsyy slsyy requested a review from UpcraftLP May 13, 2024 07:41
slsyy added 2 commits May 15, 2024 20:07
@slsyy
write tests for /account/*
2014317
@slsyy
review feedback
c1d2a44 
* skip tests on windows
* move db consts at the top of the file
* print connection string for debugging
* verify future (ideal) status codes&messages instead of the current
  wrong stuff
@ThisIsAreku ThisIsAreku mentioned this pull request May 21, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@UpcraftLP UpcraftLP Awaiting requested review from UpcraftLP

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@slsyy @UpcraftLP