chore: Refactors Acceptance Tests flow logic #1618

lantoli · 2023-11-12T20:44:11Z

Description

Refactors Acceptance Tests flow logic so info about a resource test keeps together.

Pros

Information about a test keeps together like the test regular expression and files to detect changes. No need to change/add in 3 different places for testing a resource.
Reduce duplicated code

Cons

It's harder to see what tests were skipped
All jobs are run and their containers created even if not needed, although the time-consuming steps are skipped if not needed.

Link to any related issue(s):

Type of change:

Bug fix (non-breaking change which fixes an issue). Please, add the "bug" label to the PR.
New feature (non-breaking change which adds functionality). Please, add the "enhancement" label to the PR.
Breaking change (fix or feature that would cause existing functionality to not work as expected). Please, add the "breaking change" label to the PR.
This change requires a documentation update
Documentation fix/enhancement

Required Checklist:

I have signed the MongoDB CLA
I have read the contribution guidelines
I have added tests that prove my fix is effective or that my feature works per HashiCorp requirements
I have added any necessary documentation (if appropriate)
I have run make fmt and formatted my code
If changes include deprecations or removals, I defined an isolated PR with a relevant title as it will be used in the auto-generated changelog.

Further comments

marcosuma · 2023-11-13T11:17:08Z

mongodbatlas/resource_mongodbatlas_search_index.go

@@ -485,7 +485,7 @@ func unmarshalSearchIndexAnalyzersFields(mappingString string) []admin.ApiAtlasF
 	}
 	var fields []admin.ApiAtlasFTSAnalyzers
 	if err := json.Unmarshal([]byte(mappingString), &fields); err != nil {
-		log.Printf("[ERROR] cannot unmarshal search index mapping fields: %v", err)
+		log.Printf("[ERROR] can not unmarshal search index mapping fields: %v", err)


how important is this change? should we just leave it like that? because eventually I don't think this makes it "better english" 😄

this was to test that file change detection was working fine :-) i'll revert it

here I changed a file, for generic I added PR label run-testacc-generic so i test different ways to trigger the tests

marcosuma · 2023-11-13T11:18:31Z

.github/workflows/acceptance-tests.yml

+env:
+    FORCE_TEST_ACC: ${{ github.event_name == 'workflow_dispatch' || github.event_name == 'schedule' || github.event.label.name == 'run-testacc' || inputs.parent-event-name == 'release' }}
+    TF_LOG: ${{ vars.LOG_LEVEL }}
+    SKIP_TEST_EXTERNAL_CREDENTIALS: ${{ vars.SKIP_TEST_EXTERNAL_CREDENTIALS }}
+    ACCTEST_TIMEOUT: ${{ vars.ACCTEST_TIMEOUT }}
+    PARALLEL_GO_TEST: "20"


are these "global" env?

they are passed to all jobs

are we sure this is a good idea? Here is where I would like to see all environment variables as close as possible to the job/step so I don't have to go up and down in the file to see the context.

we can decide what we want to have closer and which one we want for all.
for instances in the beginning I was going to delete PARALLEL_GO_TEST and ACCTEST_TIMEOUT because they have their default values, but in this way you can change the var instead of having to change code.

about FORCE_TEST_ACC I think it's a good idea to have it here as this doesn't depend on a specific test. For example we want to run tests always if it's triggered manually (workflow_dispatch) or if it's a release

i don't see a big problem in having common vars here. We can move them later if we realize that we want different values for different tests

andreaangiolillo · 2023-11-13T12:14:31Z

.github/workflows/acceptance-tests.yml

+  search_index:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4


Could we move actions/checkout@v4 to github/actions/acceptance-test-action/action.yml??

no, that was my first try but it can't find the action file

andreaangiolillo · 2023-11-13T12:18:56Z

.github/actions/acceptance-test-action/action.yml

+    - uses: dorny/paths-filter@v2
+      id: filter
+      with:
+        filters: ${{ inputs.filters }}


Something to keep in mind with this approach is that we call dorny/paths-filter@v2 for every job. Before we were calling dorny/paths-filter@v2 once, and then use the result of that step to run the correct jobs
.

One cons of this approach is that we will always create a run job to run every acceptance test, the job will then terminate once dorny/paths-filter@v2 is run. Right now the job is skipped instead. (Could you confirm this?)

I would suggest you to add pros/cons of this approach in the description of your PR to help the discussion

(not strong opinion here I just want to highlight this new behaviour)

you're right the jobs will be created, but the tests steps will be skipped if not need to run so at the end I think it'll be equivalent. Also it's true that we call path-filters multiple times but I suppose this action internally has to do the code for each filter group so at then end it does a similar job.

one pro of current version is that it's easier to see what acceptance tests were skipped.

i can change the description to add pros and cons

andreaangiolillo

I would add pros/cons of the proposed approach in the description of the PR to help the discussion

This reverts commit 705a0ea.

github-actions · 2023-11-15T21:01:29Z

Package	Line Rate	Health
github.com/mongodb/terraform-provider-mongodbatlas/mongodbatlas	2%	❌
github.com/mongodb/terraform-provider-mongodbatlas/mongodbatlas/framework/validator	68%	➖
github.com/mongodb/terraform-provider-mongodbatlas/mongodbatlas/testutils	37%	❌
github.com/mongodb/terraform-provider-mongodbatlas/mongodbatlas/util	17%	❌
Summary	3% (279 / 10320)	❌

lantoli changed the title ~~local action example~~ chore: local action example Nov 12, 2023

lantoli added the run-testacc-generic label Nov 13, 2023

marcosuma reviewed Nov 13, 2023

View reviewed changes

andreaangiolillo reviewed Nov 13, 2023

View reviewed changes

lantoli changed the title ~~chore: local action example~~ chore: Refactor Acceptance Tests flow logic Nov 13, 2023

lantoli changed the title ~~chore: Refactor Acceptance Tests flow logic~~ chore: Refactors Acceptance Tests flow logic Nov 13, 2023

lantoli added 15 commits November 15, 2023 19:45

local action example

b79a4c7

don't change dir

94cd3ee

pass filters

8ea5a9d

filter in new line

2535472

pass full filter

347e821

change to trigger tests

705a0ea

remove permissions section

77cdb99

pass testRegex

d0a454d

delete old job

1d925e8

simplify force test

ff300c6

undo var removal

bd7aef3

move vars

c5ea9c0

remove search index leftovers

0ec77c4

change generic tests

ff34532

update doc

228d085

lantoli force-pushed the action_acceptance_tests branch from ead0dc3 to 228d085 Compare November 15, 2023 18:46

lantoli added 5 commits November 15, 2023 19:46

Revert "change to trigger tests"

bd68a42

This reverts commit 705a0ea.

move backup_online_archive

272b5f0

show var names

54fc085

print params in local action

7fc9bb7

remove filters from echo

03ae104

lantoli removed the run-testacc-generic label Nov 16, 2023

lantoli closed this Nov 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Refactors Acceptance Tests flow logic #1618

chore: Refactors Acceptance Tests flow logic #1618

lantoli commented Nov 12, 2023 •

edited

Loading

marcosuma Nov 13, 2023

lantoli Nov 13, 2023 •

edited

Loading

lantoli Nov 13, 2023 •

edited

Loading

marcosuma Nov 13, 2023

lantoli Nov 13, 2023

marcosuma Nov 13, 2023

lantoli Nov 13, 2023 •

edited

Loading

lantoli Nov 13, 2023

andreaangiolillo Nov 13, 2023

lantoli Nov 13, 2023 •

edited

Loading

andreaangiolillo Nov 13, 2023 •

edited

Loading

lantoli Nov 13, 2023

andreaangiolillo left a comment

github-actions bot commented Nov 15, 2023

chore: Refactors Acceptance Tests flow logic #1618

chore: Refactors Acceptance Tests flow logic #1618

Conversation

lantoli commented Nov 12, 2023 • edited Loading

Description

Pros

Cons

Type of change:

Required Checklist:

Further comments

marcosuma Nov 13, 2023

Choose a reason for hiding this comment

lantoli Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

lantoli Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

marcosuma Nov 13, 2023

Choose a reason for hiding this comment

lantoli Nov 13, 2023

Choose a reason for hiding this comment

marcosuma Nov 13, 2023

Choose a reason for hiding this comment

lantoli Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

lantoli Nov 13, 2023

Choose a reason for hiding this comment

andreaangiolillo Nov 13, 2023

Choose a reason for hiding this comment

lantoli Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

andreaangiolillo Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

lantoli Nov 13, 2023

Choose a reason for hiding this comment

andreaangiolillo left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 15, 2023

lantoli commented Nov 12, 2023 •

edited

Loading

lantoli Nov 13, 2023 •

edited

Loading

lantoli Nov 13, 2023 •

edited

Loading

lantoli Nov 13, 2023 •

edited

Loading

lantoli Nov 13, 2023 •

edited

Loading

andreaangiolillo Nov 13, 2023 •

edited

Loading