PR: Reduce test workflow for each PR/commit #1780

bigtedde · 2023-08-17T23:38:52Z

Description

#1778
#1754
@m3nu , @jetchirag , @real-yfprojects

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have read the CONTRIBUTING guide.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

I provide my contribution under the terms of the license of this repository and I affirm the Developer Certificate of Origin.

bigtedde · 2023-08-17T23:40:59Z

.github/workflows/test.yml

@@ -26,16 +26,59 @@ jobs:
        shell: bash
        run: make lint

+  prepare-unit-test-matrix:


Prepares matrix's based on push/pull request vs workflow dispatch. Note: matrix's are the same for unit tests, but defined separately in case changes need to be made in the future.

bigtedde · 2023-08-17T23:42:23Z

.github/workflows/test.yml

-        borg-version: ["1.1.18", "1.2.2", "1.2.4", "2.0.0b5"]
-        exclude:
-          - borg-version: "2.0.0b5"
-            python-version: "3.8"


Biggest hurdle I ran into with the integration tests is excluding the combination of Borg 2.0.0b5 with python 3.8. Skipped this for now.

Doesn't adding the exclude to the matrix work?

Unfortunately it does not work with the way I'm dynamically creating the matrix, this only works when the matrix is static I believe.

bigtedde · 2023-08-17T23:44:03Z

Overall, this works as intended with 2 exceptions:

excluding combination of Borg 2.0.0b5 with python 3.8
received a deprecation warning:

The `set-output` command is deprecated and will be disabled soon. 
Please upgrade to using Environment Files. 
For more information see: https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/

bigtedde · 2023-08-17T23:48:20Z

With this matrix, all jobs completed in 7m 57s

.github/workflows/test.yml

bigtedde · 2023-08-19T03:00:32Z

Welp, a few trial and errors later, and I think this is in a good spot. I made a script that generates the matrices for unit and integration tests based on the type of request (PR, push, workflow dispatch) and this script can be changed easily if we want to tinker in the future. Furthermore, I got past the deprecation warning that I was having before.

The only remaining issue is that I am unable to find a way to exclude the combination of Borg v2.0.05b with python 3.8

real-yfprojects · 2023-08-19T08:18:47Z

.github/scripts/generate-matrix.sh

@@ -0,0 +1,10 @@
+event_name="$1"
+
+if [[ "$event_name" == "push" ]] || [[ "$event_name" == "pull_request" ]]; then


For a push to master I would run all tests.

Agree. That would mostly automate it.

only run a few select tests for PR commits.

run all tests when merging into master.

Okay, I have updated the matrix script to first check "is it a manual workflow dispatch OR a request from master branch" and if so fun all tests. In all other cases (push/PR/non master) run the subset of tests.

bigtedde · 2023-08-19T19:27:33Z

.github/workflows/test.yml

      - name: Setup tmate session
        uses: mxschmitt/action-tmate@v3
-        if: ${{ github.event_name == 'workflow_dispatch' && github.event.inputs.debug_enabled }}
+        if: ${{ github.event_name == 'workflow_dispatch' && github.event.inputs.debug_enabled == 'true' }}


This corrects a serious error I found while testing. Before, the conditional 'debug_enabled' would always evaluate to True since the call returns a string which is truthy. This was causing a manual workflow to always be ran in debugging mode, even with debugging disabled.

Another thing I found when running in debug mode is that the tests would always timeout (execution time > 20 minutes). Should we consider raising the execution timeout if debug mode is enabled?

I think that's because of the remote shell created. It stops execution and allows you to connect to the box.

Okay, guess the timeout is fine as is then.

…precation warnings

bigtedde · 2023-08-20T00:13:00Z

Okay, everything now works as expected and without any deprecation warnings. I implemented a new way of doing the "exclude" by creating a step that validates the matrix combinations. Although it may appear slightly clunky now, if we need to add future excluded combinations this step will be easy to edit/maintain.

real-yfprojects · 2023-08-20T07:42:36Z

.github/scripts/generate-matrix.sh

+
+if [[ "$event_name" == "workflow_dispatch" ]] || [[ "$branch_name" == "master" ]]; then
+    echo '{"python-version": ["3.8", "3.9", "3.10", "3.11"], "os": ["ubuntu-latest", "macos-latest"], "borg-version": ["1.2.4"]}' > matrix-unit.json
+    echo '{"python-version": ["3.8", "3.9", "3.10", "3.11"], "os": ["ubuntu-latest", "macos-latest"], "borg-version": ["1.1.18", "1.2.2", "1.2.4", "2.0.0b5"]}' > matrix-integration.json


Did you write the exclude like that when testing:

Suggested change

echo '{"python-version": ["3.8", "3.9", "3.10", "3.11"], "os": ["ubuntu-latest", "macos-latest"], "borg-version": ["1.1.18", "1.2.2", "1.2.4", "2.0.0b5"]}' > matrix-integration.json

echo '{"python-version": ["3.8", "3.9", "3.10", "3.11"], "os": ["ubuntu-latest", "macos-latest"], "borg-version": ["1.1.18", "1.2.2", "1.2.4", "2.0.0b5"], "exclude": [{"borg-version":"2.0.0b5, "python-version":"3.8"}]}' > matrix-integration.json

@real-yfprojects This worked like a charm :)

m3nu · 2023-08-21T15:50:20Z

Anything missing here before merging? Could we limit the "short" integration tests to Linux only?

bigtedde · 2023-08-21T17:41:10Z

Anything missing here before merging? Could we limit the "short" integration tests to Linux only?

I just made that change to the matrix script, and marked this PR as ready for review.

m3nu · 2023-08-21T19:24:15Z

Thanks a lot for taking care, @bigtedde . Will merge this and then we see how it goes in practice.

bigtedde marked this pull request as draft August 17, 2023 23:38

bigtedde commented Aug 17, 2023

View reviewed changes

m3nu reviewed Aug 18, 2023

View reviewed changes

.github/workflows/test.yml Outdated Show resolved Hide resolved

real-yfprojects reviewed Aug 19, 2023

View reviewed changes

bigtedde commented Aug 19, 2023

View reviewed changes

bigtedde added 3 commits August 19, 2023 17:03

unit and integration matrix created dynamically, exclude works, no de…

7e97ade

…precation warnings

FIXED: faulty debug logic always runs tmate session on manual workflow

518fe3f

cleanup + exclude working as intended

0975b11

bigtedde force-pushed the ci branch from a13b92b to 0975b11 Compare August 20, 2023 00:05

real-yfprojects reviewed Aug 20, 2023

View reviewed changes

bigtedde added 3 commits August 20, 2023 14:18

new exclude

df33623

made matrix script easier to read

29a7727

corrected json formatting

9655633

'short' integration test matrix os=linux only

8f94ad3

bigtedde marked this pull request as ready for review August 21, 2023 17:40

m3nu approved these changes Aug 21, 2023

View reviewed changes

m3nu merged commit 567a354 into borgbase:master Aug 21, 2023

bigtedde deleted the ci branch August 21, 2023 19:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR: Reduce test workflow for each PR/commit #1780

PR: Reduce test workflow for each PR/commit #1780

bigtedde commented Aug 17, 2023 •

edited

Loading

bigtedde Aug 17, 2023

bigtedde Aug 17, 2023

real-yfprojects Aug 19, 2023

bigtedde Aug 19, 2023 •

edited

Loading

bigtedde commented Aug 17, 2023

bigtedde commented Aug 17, 2023

bigtedde commented Aug 19, 2023

real-yfprojects Aug 19, 2023

m3nu Aug 19, 2023 •

edited

Loading

bigtedde Aug 19, 2023

bigtedde Aug 19, 2023 •

edited

Loading

bigtedde Aug 20, 2023

real-yfprojects Aug 20, 2023

bigtedde Aug 21, 2023

bigtedde commented Aug 20, 2023

real-yfprojects Aug 20, 2023

bigtedde Aug 21, 2023

m3nu commented Aug 21, 2023

bigtedde commented Aug 21, 2023

m3nu commented Aug 21, 2023

		@@ -0,0 +1,10 @@
		event_name="$1"

		if [[ "$event_name" == "push" ]] \|\| [[ "$event_name" == "pull_request" ]]; then

	echo '{"python-version": ["3.8", "3.9", "3.10", "3.11"], "os": ["ubuntu-latest", "macos-latest"], "borg-version": ["1.1.18", "1.2.2", "1.2.4", "2.0.0b5"]}' > matrix-integration.json
	echo '{"python-version": ["3.8", "3.9", "3.10", "3.11"], "os": ["ubuntu-latest", "macos-latest"], "borg-version": ["1.1.18", "1.2.2", "1.2.4", "2.0.0b5"], "exclude": [{"borg-version":"2.0.0b5, "python-version":"3.8"}]}' > matrix-integration.json

PR: Reduce test workflow for each PR/commit #1780

PR: Reduce test workflow for each PR/commit #1780

Conversation

bigtedde commented Aug 17, 2023 • edited Loading

Description

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigtedde Aug 19, 2023 • edited Loading

Choose a reason for hiding this comment

bigtedde commented Aug 17, 2023

bigtedde commented Aug 17, 2023

bigtedde commented Aug 19, 2023

Choose a reason for hiding this comment

m3nu Aug 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigtedde Aug 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigtedde commented Aug 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m3nu commented Aug 21, 2023

bigtedde commented Aug 21, 2023

m3nu commented Aug 21, 2023

bigtedde commented Aug 17, 2023 •

edited

Loading

bigtedde Aug 19, 2023 •

edited

Loading

m3nu Aug 19, 2023 •

edited

Loading

bigtedde Aug 19, 2023 •

edited

Loading