Skip to content

Write tests for account#12

Open
slsyy wants to merge 2 commits intopagefaultgames:masterfrom
slsyy:write-tests-for-account
Open

Write tests for account#12
slsyy wants to merge 2 commits intopagefaultgames:masterfrom
slsyy:write-tests-for-account

Conversation

@slsyy
Copy link
Copy Markdown
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

@slsyy slsyy force-pushed the write-tests-for-account branch from cd45b6e to 9db7fa5 Compare May 11, 2024 22:40
UpcraftLP
UpcraftLP suggested changes May 11, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@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.

@slsyy
Copy link
Copy Markdown
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
Copy Markdown
Contributor

UpcraftLP commented May 11, 2024

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
Copy Markdown
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 force-pushed the write-tests-for-account branch from 9db7fa5 to 81a01aa Compare May 12, 2024 11:53
@slsyy slsyy requested a review from UpcraftLP May 13, 2024 07:41
@slsyy slsyy force-pushed the write-tests-for-account branch from 81a01aa to df971a2 Compare May 13, 2024 14:00
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
@slsyy slsyy force-pushed the write-tests-for-account branch from df971a2 to c1d2a44 Compare May 15, 2024 18:56
@ThisIsAreku ThisIsAreku mentioned this pull request May 21, 2024
Closed
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