chore: ensure all tests are run #1261
No reviewers
mfenniak
Labels
No labels
FreeBSD
Kind/Breaking
Kind/Bug
Kind/Chore
Kind/DependencyUpdate
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Windows
linux-powerpc64le
linux-riscv64
linux-s390x
run-end-to-end-tests
run-forgejo-tests
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
forgejo/runner!1261
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "aahlenst/runner:run-all-tests"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
The Forgejo Runner Makefile only looks for tests in some directories. If someone adds a test in another place, they have to remember to update the Makefile. That is error-prone and easy to forget. Another problem is that integration tests are stored alongside short tests and not in a separate directory tree. That makes it hard to separate them.
In response to #1255 (comment), all
go testcommands now scan the entire source tree for tests. All integration tests are skipped iftesting.Short()istrue. Whilemake testruns only short tests,make integration-testnow runs all tests, no matter whether they are short or not. Separating them would either require the introduction of test suites or moving them to a separate directory tree.CI was restructured to reflect the changes. The build step no longer runs any tests because the integration test step will run all tests anyway. CI might run slightly longer than before because short tests and some integration tests that do not require a container runtime are now run twice with each Docker version instead of once.
Additional slow tests were identified and marked as slow by running
go test -json -short ./... | gotestsum tool slowest --threshold 200ms. Some were reclassified after a manual review.c2bceca816e92b36db0be92b36db0b26553b9cea26553b9ceafe25cdd00efe25cdd00e0139187d8d0139187d8d2d17f4c3442d17f4c3446b640ce48d53e1d9b9b6bc77fe2ad9bc77fe2ad9f54d282d93@mfenniak I could use some help. For some reason, the test runs fail, seemingly for no reason. There's no
FAILin the entire logs. It abruptly ends withmake: *** [Makefile:113: integration-test] Error 1. I cannot reproduce it locally. Do you have an idea? Apart from that, it should be ready.It's possible that this is being affected by #1258, and part of the logs are being lost. I pulled your branch into my LXC test environment and was able to get this concrete error out. This is probably caused by the change in
FORGEJO_RUNNER_SECRETin thetest.ymlworkflow?Thanks a lot. That helps.
@mfenniak wrote in #1261 (comment):
Yes, it is. I passed the runner registration token instead of the offline registration token 🤦 Let's see whether I can create an offline registration token with
setup-forgejo. I cannot reuse the previously used service container because it doesn't work with LXC. And separate steps don't work with the new test arrangement.setup-forgejoseems not to allow me to accessforgejoto create an offline registration token. After reading #1268 (comment) and considering that we will need a lot more testing for all the new commands, ideally with different versions of Forgejo, I plan on skippinginternal/app/cmd/create-runner-file_test.goso that we can proceed here. Afterwards, I'll turninternal/app/cmd/create-runner-file_test.gointo proper end-to-end tests.6bde97ab725d95da1c3c5d95da1c3cbc576da7d0bc576da7d031b134c09231b134c092711434e1fa711434e1fa3db3bad65fWIP: chore: ensure all tests are runto chore: ensure all tests are run@ -7,3 +7,3 @@- run: |echo "forgejo.ref=${{ forgejo.ref }}"[[ "${{ forgejo.ref }}" =~ ^refs/(heads|tags)/.+$ ]] || exit 1[[ "${{ forgejo.ref }}" =~ ^refs/(heads|remotes|rewritten|tags)/.+$ ]] || exit 1Hm... where does this change come from?
When I originally added the test, I didn't include all kinds of refs. I added those now because a PR looks like
refs/remotes/pull/1261/head. Because not all tests were run before, and that's one of those that have not been run, we never noticed that the test was broken.Thank you for taking this on! 🙂 I've tweaked the repo's branch protection rules to match the update.