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

Fix incorrect test code for error handling #27139

Merged
merged 2 commits into from
Sep 20, 2023
Merged

Fix incorrect test code for error handling #27139

merged 2 commits into from
Sep 20, 2023

Conversation

wxiaoguang
Copy link
Contributor

If assert.NoError fails, it should return. Otherwise the code could still execute even if the error occurs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 19, 2023
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 19, 2023
@delvh
Copy link
Member

delvh commented Sep 19, 2023

But if assert.NoError fails, the test will already be marked as failed, correct?

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Sep 19, 2023 •
edited
Loading

But if assert.NoError fails, the test will already be marked as failed, correct?

Yes. But there could be no different no matter the "defer" comes before or after if there is no return, the code could still execute to defer nil.Close() if the assert fails.

I do not consider it as a serious problem in test code. But since 27129 has been merged, I think it's better to show the right approach.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Sep 19, 2023
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Sep 19, 2023
@puni9869 puni9869 added skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/testing reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels Sep 19, 2023
@puni9869
Copy link
Member

🚀

@KN4CK3R
Copy link
Member

KN4CK3R commented Sep 19, 2023

This code is cleaner than the panic but we would need to rewrite lots of tests. At the moment we don't evade panics and I see not much value in doing so. The code in this PR for example should check the returned repo with assert.NotNil too. Here we would need the return too. And for every slice length check and so on. That will just bloat our tests.

It would be better to use require which instantly fails and aborts execution of the test. But for that we still would need to rewrite lots of tests.

@KN4CK3R KN4CK3R removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 19, 2023
@wxiaoguang
Copy link
Contributor Author

At the moment we don't evade panics and I see not much value in doing so.

Agree with it (also my comment #27139 (comment): I do not consider it as a serious problem in test code. But since 27129 has been merged, I think it's better to show the right approach.)

In some cases, if assert is helpful, while for other cases, it's safe to leave the panic in the test.

@wxiaoguang
Copy link
Contributor Author

I think we can merged this. If it doesn't look good or need future improvements, feel free to revert and/or propose new fixes.

@wxiaoguang wxiaoguang merged commit 2814f2f into go-gitea:main Sep 20, 2023
25 checks passed
@GiteaBot GiteaBot added this to the 1.22.0 milestone Sep 20, 2023
@wxiaoguang wxiaoguang deleted the fix-wiki-test branch September 20, 2023 00:51
zjjhot added a commit to zjjhot/gitea that referenced this pull request Sep 20, 2023
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
add8afe 
* giteaofficial/main:
  Improve actions docs related to `pull_request` event (go-gitea#27126)
  Remove outdated paragraphs when comparing Gitea Actions to GitHub Actions (go-gitea#27119)
  Fix: treat tab "overview" as "repositories" in user profiles without readme (go-gitea#27124)
  Fix incorrect test code for error handling (go-gitea#27139)
  Increase auth provider icon size on login page (go-gitea#27122)
  fix pagination for followers and following (go-gitea#27127)
  services/wiki: Close() after error handling (go-gitea#27129)
  Use fetch helpers instead of fetch (go-gitea#27026)
  Change green buttons to primary color (go-gitea#27099)
  Fix wrong xorm get usage on migration (go-gitea#27111)
  Fix the incorrect route path in the user edit page. (go-gitea#27007)
  Refactor lfs requests (go-gitea#26783)
  Display archived labels specially when listing labels (go-gitea#26820)
  Remove a `gt-float-right` and some unnecessary helpers (go-gitea#27110)
  [skip ci] Updated licenses and gitignores
  Fix token endpoints ignore specified account (go-gitea#27080)
  Make SSPI auth mockable (go-gitea#27036)
@delvh delvh modified the milestones: 1.22.0, 1.21.0 Sep 20, 2023
silverwind added a commit to silverwind/gitea that referenced this pull request Sep 21, 2023
@silverwind
Merge remote-tracking branch 'origin/main' into vitest-global
d8ee8ac 
* origin/main:
  Fix dropdown icon position (go-gitea#27175)
  Fix repo sub menu (go-gitea#27169)
  Fix review request number and add more tests (go-gitea#27104)
  Fix the variable regexp pattern on web page (go-gitea#27161)
  Fix organization field being null in POST /orgs/{orgid}/teams (go-gitea#27150)
  Add index to `issue_user.issue_id` (go-gitea#27154)
  [skip ci] Updated translations via Crowdin
  Start development on Gitea 1.22 (go-gitea#27155)
  Fix successful return value for `SyncAndGetUserSpecificDiff` (go-gitea#27152)
  Improve actions docs related to `pull_request` event (go-gitea#27126)
  Remove outdated paragraphs when comparing Gitea Actions to GitHub Actions (go-gitea#27119)
  Fix: treat tab "overview" as "repositories" in user profiles without readme (go-gitea#27124)
  Fix incorrect test code for error handling (go-gitea#27139)
  Increase auth provider icon size on login page (go-gitea#27122)
  fix pagination for followers and following (go-gitea#27127)
  services/wiki: Close() after error handling (go-gitea#27129)
  Use fetch helpers instead of fetch (go-gitea#27026)
  Change green buttons to primary color (go-gitea#27099)
  Fix wrong xorm get usage on migration (go-gitea#27111)
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Dec 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@puni9869 puni9869 puni9869 approved these changes

@delvh delvh delvh approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/testing
Projects
None yet
Milestone
1.21.0
Development

Successfully merging this pull request may close these issues.
5 participants
@wxiaoguang @delvh @puni9869 @KN4CK3R @GiteaBot