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

Add artifacts test fixture #30300

Merged
merged 5 commits into from
Nov 1, 2024
Merged

Add artifacts test fixture #30300

merged 5 commits into from
Nov 1, 2024

Conversation

kdumontnu
Copy link
Contributor

@kdumontnu kdumontnu commented Apr 5, 2024 •
edited
Loading

Closes #30296

  • Adds a DB fixture for actions artifacts
  • Adds artifacts test files
  • Clears artifacts test files between each run
    • Note: I initially initialized the artifacts only for artifacts tests, but because the files are small it only takes ~8ms, so I changed it to always run in test setup for simplicity
  • Fix some otherwise flaky tests by making them not depend on previous tests

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 5, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 5, 2024
@kdumontnu kdumontnu requested a review from fuxiaohei April 5, 2024 20:57
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Apr 5, 2024
@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 5, 2024
@kdumontnu kdumontnu marked this pull request as draft April 5, 2024 21:19
@kdumontnu kdumontnu force-pushed the kd/artifacts_test_fixture branch 2 times, most recently from cee2e82 to e4dfaad Compare October 31, 2024 14:23
@kdumontnu kdumontnu marked this pull request as ready for review October 31, 2024 14:24
@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 Oct 31, 2024
@kdumontnu kdumontnu force-pushed the kd/artifacts_test_fixture branch from 4901ecc to 1296ec6 Compare October 31, 2024 22:34
@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 Nov 1, 2024
@lunny lunny added type/testing reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels Nov 1, 2024
@lunny lunny merged commit 66971e5 into go-gitea:main Nov 1, 2024
26 checks passed
@GiteaBot GiteaBot added this to the 1.24.0 milestone Nov 1, 2024
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Nov 1, 2024
@wxiaoguang
Copy link
Contributor

A following up PR: Refactor tests to prevent from unnecessary preparations #32398

@kdumontnu
Copy link
Contributor Author

A following up PR: Refactor tests to prevent from unnecessary preparations #32398

Most of the other preparations are very lightweight. The PrepareArtifactsStorage only takes a couple of milliseconds and doesn’t impact the overall time, so I figured it was best to keep it simple.

@wxiaoguang
Copy link
Contributor

A following up PR: Refactor tests to prevent from unnecessary preparations #32398

Most of the other preparations are very lightweight. The PrepareArtifactsStorage only takes a couple of milliseconds and doesn’t impact the overall time, so I figured it was best to keep it simple.

I agree, but we could make the situation better and we should simplify the PrepareTestEnv function, it was patched a lot and some mechanisms there are not ideal.

And, when I wrote PRs, maintainers forced me to rewrite var x []int to x := make([]int, 0, len(xxx)), maybe the difference was only nano-second level.

For this case, I think my refactoring is still simple enough, while it makes the PrepareTestEnv clearer than before.

So would you agree to approve my PR?

@kdumontnu
Copy link
Contributor Author

A following up PR: Refactor tests to prevent from unnecessary preparations #32398

Most of the other preparations are very lightweight. The PrepareArtifactsStorage only takes a couple of milliseconds and doesn’t impact the overall time, so I figured it was best to keep it simple.

I agree, but we could make the situation better and we should simplify the PrepareTestEnv function, it was patched a lot and some mechanisms there are not ideal.

And, when I wrote PRs, maintainers forced me to rewrite var x []int to x := make([]int, 0, len(xxx)), maybe the difference was only nano-second level.

For this case, I think my refactoring is still simple enough, while it makes the PrepareTestEnv clearer than before.

So would you agree to approve my PR?

Yeah, agreed. Was on my phone earlier so couldn't fully review. Looking at your PR now.

@lunny lunny modified the milestones: 1.24.0, 1.23.0 Nov 2, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request Nov 6, 2024
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
f4c5144 
* giteaofficial/main: (21 commits)
  Use 8 as default value for git lfs concurrency (go-gitea#32421)
  Fix milestone deadline and date related problems (go-gitea#32339)
  Only query team tables if repository is under org when getting assignees (go-gitea#32414)
  Refactor RepoRefByType (go-gitea#32413)
  Refactor template ctx and render utils (go-gitea#32422)
  Refactor DateUtils and merge TimeSince (go-gitea#32409)
  Refactor markup package (go-gitea#32399)
  Add some handy markdown editor features (go-gitea#32400)
  Make LFS http_client parallel within a batch. (go-gitea#32369)
  Refactor repo legacy (go-gitea#32404)
  Replace DateTime with proper functions (go-gitea#32402)
  Fix git error handling (go-gitea#32401)
  Fix created_unix for mirroring (go-gitea#32342)
  Replace DateTime with DateUtils (go-gitea#32383)
  improve performance of diffs (go-gitea#32393)
  Refactor tests to prevent from unnecessary preparations (go-gitea#32398)
  Add artifacts test fixture (go-gitea#30300)
  Fix `missing signature key` error when pulling Docker images with `SERVE_DIRECT` enabled (go-gitea#32365)
  Fix a number of typescript issues (go-gitea#32308)
  Update go dependencies (go-gitea#32389)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lunny lunny lunny approved these changes

@bohde bohde bohde approved these changes

@fuxiaohei fuxiaohei Awaiting requested review from fuxiaohei

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/go Pull requests that update Go code size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/testing
Projects
None yet
Milestone
1.23.0
Development

Successfully merging this pull request may close these issues.

Artifact Integration Tests Not Properly Isolated
5 participants
@kdumontnu @wxiaoguang @bohde @lunny @GiteaBot