Skip to content

test(bigquery-jdbc): add test tags & ability to skip them - #13916

Merged
logachev merged 7 commits into
mainfrom
kirl/tag_tests
Jul 28, 2026
Merged

test(bigquery-jdbc): add test tags & ability to skip them#13916
logachev merged 7 commits into
mainfrom
kirl/tag_tests

Conversation

@logachev

Copy link
Copy Markdown
Contributor

No description provided.

@logachev
logachev marked this pull request as ready for review July 27, 2026 22:45
@logachev
logachev requested review from a team as code owners July 27, 2026 22:45

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces support for excluding specific test groups or tags via an excludedGroups variable in the Makefile and annotates several integration tests with @Tag("advanced") and @Tag("known_issue"). The review feedback highlights multiple instances in the Makefile where excludedGroups is passed unquoted to $(MAKE) or within Docker arguments. If the variable contains spaces, this will cause word splitting and lead to build or execution failures; wrapping these references in quotes is recommended to ensure robust execution.

Comment thread java-bigquery-jdbc/Makefile Outdated
Comment thread java-bigquery-jdbc/Makefile Outdated
Comment thread java-bigquery-jdbc/Makefile Outdated
Comment thread java-bigquery-jdbc/Makefile Outdated
Comment thread java-bigquery-jdbc/Makefile Outdated
@logachev

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request adds support for excluding specific test tags or groups (such as 'advanced' and 'known_issue') during test execution by introducing an excludedGroups variable in the Makefile and tagging various integration tests. The feedback highlights a potential issue where passing an empty -DexcludedGroups parameter to Maven when the variable is unset can cause JUnit 5 to throw an IllegalArgumentException. To resolve this, it is recommended to conditionally define a helper variable in the Makefile so that the property is only passed to Maven when excludedGroups is explicitly provided.

Comment on lines +9 to +12
excludedGroups ?=
comma := ,
empty :=
space := $(empty) $(empty)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Passing an empty -DexcludedGroups="" to Maven Surefire/Failsafe when excludedGroups is not specified (which is the default) can cause JUnit 5 to throw an IllegalArgumentException (e.g., Tag name must not be null or empty or Invalid tag expression).

To prevent this and ensure the default build runs successfully, we can conditionally define a helper variable EXCLUDED_GROUPS_PROP in the Makefile using GNU Make's $(if ...) function, and only pass the -DexcludedGroups property to Maven when excludedGroups is actually set.

excludedGroups ?=
EXCLUDED_GROUPS_PROP = $(if $(excludedGroups),-DexcludedGroups="$(excludedGroups)",)
comma := ,
empty :=
space := $(empty) $(empty)
References
  1. When replacing a hardcoded default environment variable with a Makefile variable, verify whether an empty or unset value will break the execution, or confirm that the application handles an empty string identically to the default value.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Verified that it works

Comment thread java-bigquery-jdbc/Makefile
Comment thread java-bigquery-jdbc/Makefile
@logachev
logachev merged commit d091863 into main Jul 28, 2026
206 checks passed
@logachev
logachev deleted the kirl/tag_tests branch July 28, 2026 19:56
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