Adds integration tests for mysql, postgres and go client-server. #72

FedeNQ · 2023-06-15T15:36:12Z

Changes proposal:

Adds integration test for mysql, postgres and client-server golang app.

Signed-off-by: JU4N98 <[email protected]>

faisal-memon · 2023-06-18T05:29:25Z

Thanks @FedeNQ for writing all these unit tests.

faisal-memon · 2023-06-18T05:31:34Z

it/docker-compose.yaml

+version: '3'
+services:
+  spire-server:
+    image: ghcr.io/spiffe/spire-server:1.6.3


Can you update to 1.7.0 for server and agent?

faisal-memon · 2023-06-18T05:42:14Z

Can you move the tests form it/ to .github/tests?

faisal-memon · 2023-06-18T05:41:01Z

.github/workflows/pr_build.yaml

+      - name: Run integration tests
+        run: ./integration_test.sh
+        shell: bash
+        working-directory: ./it


What do you think of using a test matrix here and calling each test individually? That way it will show what test is running in the Github Actions output.

This snippet from the helm-charts repo will step through the directory with all the tests and build the matrix. https://github.com/spiffe/helm-charts/blob/main/.github/workflows/helm-chart-ci.yaml#L114-L117

Signed-off-by: FedeNQ <[email protected]>

FedeNQ · 2023-06-20T19:19:11Z

Hi! Thanks for the review, we've made some changes based on the comments that you've provided.

Can you move the tests form it/ to .github/tests?

In order to add the build-matrix we've created a bunch of scripts on .github/tests that call the required scripts with parameters, should we move ALL the scripts to .github/tests or is it correct as it is now?

faisal-memon · 2023-06-28T22:41:50Z

Hi @FedeNQ

Thanks for making the requested changes.

In order to add the build-matrix we've created a bunch of scripts on .github/tests that call the required scripts with parameters, should we move ALL the scripts to .github/tests or is it correct as it is now

Yes. Having the tests there would eliminate the need for those extra scripts and you could just call the tests directly.

Also if you can add a strategy: section that would be great. Then you won't need the for loop and can just do bash .github/tests/${{ matrix.values }}.

Described here: https://docs.github.com/en/actions/using-jobs/using-a-matrix-for-your-jobs

faisal-memon · 2023-07-03T15:46:07Z

Thanks @FedeNQ . The test matrix changes look good. The restore-entry.test.sh is failing and change-entry-test.sh is getting cancelled.

FedeNQ · 2023-07-03T17:55:13Z

Thanks @FedeNQ . The test matrix changes look good. The restore-entry.test.sh is failing and change-entry-test.sh is getting cancelled.

Thanks!
Yes, it appears that there's an issue with building the container. I'm currently working on fix that problem.

* adds strategy Signed-off-by: FedeNQ <[email protected]> * changes bash command Signed-off-by: FedeNQ <[email protected]> --------- Signed-off-by: FedeNQ <[email protected]>

* Change location of test files Signed-off-by: FedeNQ <[email protected]> * Change location of restore-entry-test Signed-off-by: FedeNQ <[email protected]> * Change location of restore-entry-test Signed-off-by: FedeNQ <[email protected]> * Adds shell in Run tests Signed-off-by: FedeNQ <[email protected]> --------- Signed-off-by: FedeNQ <[email protected]>

Signed-off-by: FedeNQ <[email protected]>

FedeNQ · 2023-07-06T18:46:19Z

@faisal-memon It's seems like we had a false positive on some test, and a problem with the retries, but it should work now.
Let me know if you see another change needed!
Thanks

faisal-memon · 2023-07-07T17:31:37Z

Thanks @FedeNQ Can you move everything from it folder to .github/tests? That way you won't need those stub scripts.

FedeNQ · 2023-07-07T17:58:55Z

Can you move everything from it folder to .github/tests? That way you won't need those stub scripts.

The scripts on test folder call the scripts on IT folder with parameters, that's why they are necessary, so move them to .github/tests won't solve the problem (it will avoid only the code block used to move from test folder to IT folder)
A solution could be move all to test folder and change the way we've made the scripts, having one test per case, instead of the parametizable tests that we've, should we?

* Move IT folder to .github folder Signed-off-by: FedeNQ <[email protected]> * Add ./.github/it to working directory Signed-off-by: FedeNQ <[email protected]> * Change target_dir in tests Signed-off-by: FedeNQ <[email protected]> * Change target_dir Signed-off-by: FedeNQ <[email protected]> * Change target_dir Signed-off-by: FedeNQ <[email protected]> * Add /.github in path Signed-off-by: FedeNQ <[email protected]> * change on docker-compose Signed-off-by: FedeNQ <[email protected]> * change target_dir in run-postgres-test Signed-off-by: FedeNQ <[email protected]> --------- Signed-off-by: FedeNQ <[email protected]>

faisal-memon · 2023-07-13T17:07:09Z

Can you move everything from it folder to .github/tests? That way you won't need those stub scripts.

The scripts on test folder call the scripts on IT folder with parameters, that's why they are necessary, so move them to .github/tests won't solve the problem (it will avoid only the code block used to move from test folder to IT folder) A solution could be move all to test folder and change the way we've made the scripts, having one test per case, instead of the parametizable tests that we've, should we?

No need to rewrite the tests. Just move to .github/tests then we can merge.

FedeNQ · 2023-07-13T17:19:53Z

I've moved the IT folder to .github
Is it ok now, or should I move it to .github/tests?

faisal-memon · 2023-07-13T17:30:25Z

.github/tests since it is tests

Signed-off-by: FedeNQ <[email protected]>

faisal-memon

Thanks @FedeNQ for creating these valuable tests.

faisal-memon · 2023-07-20T15:20:27Z

@FedeNQ I just noticed integration-tests (change-entry-test.sh) is failing

Signed-off-by: FedeNQ <[email protected]>

FedeNQ · 2023-07-20T17:11:17Z

@faisal-memon I made some changes, and now it passes all checks. Would it be a good idea to re-run all jobs to double-check?

faisal-memon · 2023-07-20T17:35:36Z

@faisal-memon I made some changes, and now it passes all checks. Would it be a good idea to re-run all jobs to double-check?

Thanks, looks good now.

Adds integration tests for mysql, postgres and go client-server.

ab55868

Signed-off-by: JU4N98 <[email protected]>

FedeNQ requested review from faisal-memon and MarcosDY as code owners June 15, 2023 15:36

JU4N98 added 2 commits June 15, 2023 16:11

Fixes spiffe-helper dockerfile.

8cde35e

Signed-off-by: JU4N98 <[email protected]>

Adds integration tests to workflows.

58b4f66

Signed-off-by: JU4N98 <[email protected]>

faisal-memon reviewed Jun 18, 2023

View reviewed changes

faisal-memon self-assigned this Jun 18, 2023

faisal-memon reviewed Jun 18, 2023

View reviewed changes

JU4N98 force-pushed the add-integration-test branch from c43f7d3 to 58b4f66 Compare June 20, 2023 17:50

Adds build-matrix job

229c6d6

Signed-off-by: FedeNQ <[email protected]>

FedeNQ requested a review from faisal-memon June 21, 2023 14:03

FedeNQ and others added 6 commits July 5, 2023 10:13

Add strategy to integration test (#9)

8549afd

* adds strategy Signed-off-by: FedeNQ <[email protected]> * changes bash command Signed-off-by: FedeNQ <[email protected]> --------- Signed-off-by: FedeNQ <[email protected]>

Changes exit code in tests (#12)

179d76f

Signed-off-by: FedeNQ <[email protected]>

change on max_attempts

7da785d

Signed-off-by: FedeNQ <[email protected]>

change max_attempts on build.sh

7461a77

Signed-off-by: FedeNQ <[email protected]>

increase ttl and attemps

c4901bd

Signed-off-by: FedeNQ <[email protected]>

FedeNQ force-pushed the add-integration-test branch from 0b79565 to c4901bd Compare July 5, 2023 13:16

FedeNQ force-pushed the add-integration-test branch from adc80df to 6c29852 Compare July 12, 2023 19:28

FedeNQ added 4 commits July 13, 2023 16:22

move IT to tests folder

676e027

Signed-off-by: FedeNQ <[email protected]>

change pr_build

a79e4e1

Signed-off-by: FedeNQ <[email protected]>

Change target_dir on tests

6f9d06b

Signed-off-by: FedeNQ <[email protected]>

Change release_build.yaml

86e4f6c

Signed-off-by: FedeNQ <[email protected]>

faisal-memon approved these changes Jul 20, 2023

View reviewed changes

Merge branch 'main' into add-integration-test

60c3ace

Change ttl & retries

0c08c34

Signed-off-by: FedeNQ <[email protected]>

faisal-memon merged commit 68f73c1 into spiffe:main Jul 20, 2023
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds integration tests for mysql, postgres and go client-server. #72

Adds integration tests for mysql, postgres and go client-server. #72

FedeNQ commented Jun 15, 2023

faisal-memon commented Jun 18, 2023

faisal-memon Jun 18, 2023

faisal-memon commented Jun 18, 2023

faisal-memon Jun 18, 2023

FedeNQ commented Jun 20, 2023 •

edited

Loading

faisal-memon commented Jun 28, 2023

faisal-memon commented Jul 3, 2023

FedeNQ commented Jul 3, 2023

FedeNQ commented Jul 6, 2023

faisal-memon commented Jul 7, 2023

FedeNQ commented Jul 7, 2023

faisal-memon commented Jul 13, 2023

FedeNQ commented Jul 13, 2023

faisal-memon commented Jul 13, 2023

faisal-memon left a comment

faisal-memon commented Jul 20, 2023

FedeNQ commented Jul 20, 2023

faisal-memon commented Jul 20, 2023

Adds integration tests for mysql, postgres and go client-server. #72

Adds integration tests for mysql, postgres and go client-server. #72

Conversation

FedeNQ commented Jun 15, 2023

Changes proposal:

faisal-memon commented Jun 18, 2023

faisal-memon Jun 18, 2023

Choose a reason for hiding this comment

faisal-memon commented Jun 18, 2023

faisal-memon Jun 18, 2023

Choose a reason for hiding this comment

FedeNQ commented Jun 20, 2023 • edited Loading

faisal-memon commented Jun 28, 2023

faisal-memon commented Jul 3, 2023

FedeNQ commented Jul 3, 2023

FedeNQ commented Jul 6, 2023

faisal-memon commented Jul 7, 2023

FedeNQ commented Jul 7, 2023

faisal-memon commented Jul 13, 2023

FedeNQ commented Jul 13, 2023

faisal-memon commented Jul 13, 2023

faisal-memon left a comment

Choose a reason for hiding this comment

faisal-memon commented Jul 20, 2023

FedeNQ commented Jul 20, 2023

faisal-memon commented Jul 20, 2023

FedeNQ commented Jun 20, 2023 •

edited

Loading