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.

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

Improve test matrix comprehensiveness #54

Merged
merged 9 commits into from
Dec 18, 2023

Conversation

RomainMuller
Copy link
Contributor

@RomainMuller RomainMuller commented Dec 13, 2023

Run tests with a comprehensive cartesian product of all configurations:

  • Operating systems & architectures1
  • CGO_ENABLED or not
  • Go runtimes: 1.19, 1.20, 1.21
  • Go tags: none, datadog.no_waf, go1.222, datadog.no_waf,go1.22

This uncovered compatibility gaps in the matrix, which this PR fixes as well:

  • unsupported platform or architecture + datadog.no_waf
  • unsupported go runtime + unsupported platform

Footnotes

  1. Multiple architectures are only tested for linux platforms due to technical limitations

  2. go1.22 is a stand-in for future go releases for which purego support needs checking

@RomainMuller RomainMuller force-pushed the romain.marcadier/add-coverage-report branch 3 times, most recently from 8639479 to 8a59852 Compare December 14, 2023 10:39
@RomainMuller RomainMuller changed the title add coverage report visual improve text matrix comprehensiveness Dec 14, 2023
@RomainMuller RomainMuller force-pushed the romain.marcadier/add-coverage-report branch 5 times, most recently from 6dcfdd0 to 0608120 Compare December 14, 2023 11:09
@RomainMuller RomainMuller changed the title improve text matrix comprehensiveness Improve test matrix comprehensiveness Dec 14, 2023
@RomainMuller RomainMuller force-pushed the romain.marcadier/add-coverage-report branch from 0608120 to 051966f Compare December 14, 2023 11:14
@RomainMuller RomainMuller force-pushed the romain.marcadier/add-coverage-report branch from 051966f to 7bae7f5 Compare December 14, 2023 11:50
@RomainMuller RomainMuller marked this pull request as ready for review December 14, 2023 12:50
@RomainMuller RomainMuller requested a review from a team as a code owner December 14, 2023 12:50
Copy link
Contributor

@eliottness eliottness left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't get why you needed to change the job names, they were explicit enough I think

.github/workflows/test.yml Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
waf_manually_disabled.go Outdated Show resolved Hide resolved
waf_unsupported_target.go Outdated Show resolved Hide resolved
waf_unsupported_target_test.go Outdated Show resolved Hide resolved
@eliottness eliottness mentioned this pull request Dec 14, 2023
Comment on lines +151 to +154
docker exec -i gha-${{ github.run_id }} \
go run gotest.tools/gotestsum@latest -- \
-v -count=10 -shuffle=on -tags='${{ matrix.go-tags }}' \
./...
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually prefer a multi-stage Dockerfile where the final container image only contains the produced binary and its dependencies, without all of the dev tools. This allows us to find setup limitations/issues.

OTOH, it would be valid to say this test's role is just to execute the tests on the given containers, regardless of the setup/onboarding use-cases.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what I also think is that setup cases should be tested in the appsec test app https://github.com/DataDog/appsec-go-test-app/blob/main/Dockerfile at the very least

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd need to figure out how to pre-build test binaries and correctly execute them then... but I'm not against the idea. We'd then also want bare-metal tests to have separated build & run jobs (where run only has the test binary available).

@RomainMuller
Copy link
Contributor Author

I still don't get why you needed to change the job names, they were explicit enough I think

I largely re-made this from scratch and initially wanted to combine all flavors in a single job (but that is terribly inconvenient to mix docker & bare metal scenarios)

Copy link
Contributor

@eliottness eliottness left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@RomainMuller RomainMuller merged commit 2cbd3ff into main Dec 18, 2023
216 of 217 checks passed
@RomainMuller RomainMuller deleted the romain.marcadier/add-coverage-report branch December 18, 2023 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants