You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/02/09 06:58:38 UTC
[GitHub] [ozone] adoroszlai opened a new pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
adoroszlai opened a new pull request #3061:
URL: https://github.com/apache/ozone/pull/3061
## What changes were proposed in this pull request?
_unit_ and _integration_ checks run tests in mutually exclusive submodules. We may skip _integration_ check if only unit *tests* are changed. However, we need to run _integration_ if any helper is changed, since we don't know if it's used by integration tests or not.
We can distinguish tests and test utilities by:
* file name pattern (`Test\*.java` are tests, others are utilities) OR
* extracting unit tests into separate submodule (similar to `ratis-test` in Ratis)
The first approach is quick and dirty.
The second one needs a bit more work, but is clean and has the additional benefit that all tests can be run despite any failures in dependencies. (Currently a test failure in e.g. `hdds-common` causes build/test in `hdds-server-framework` and other modules that depend on it.) The downside is that it may cause many merge conflicts for everyone (feature branches, other in-progress work, open PRs).
I propose to do it now the first way, then do it properly after current feature branches are merged.
https://issues.apache.org/jira/browse/HDDS-6186
## How was this patch tested?
Added test cases in `selective_ci_checks.bats`.
```
bats dev-support/ci/selective_ci_checks.bats
✓ checkstyle and bats
✓ compose only
✓ compose and robot
✓ runner image update
✓ check script
✓ integration and unit: java change
✓ integration and unit: script change
✓ unit only
✓ unit helper
✓ integration only
✓ kubernetes only
✓ docs only
✓ java-only change
✓ java and compose change
✓ java and docs change
✓ pom change
✓ CI lib change
✓ CI workflow change
✓ root README
✓ ignored code
✓ other README
21 tests, 0 failures
```
https://github.com/adoroszlai/hadoop-ozone/actions/runs/1812880105
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GeorgeJahad commented on a change in pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
GeorgeJahad commented on a change in pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#discussion_r806173443
##########
File path: dev-support/ci/selective_ci_checks.sh
##########
@@ -242,18 +242,25 @@ function get_count_doc_files() {
start_end::group_end
}
-function get_count_junit_files() {
- start_end::group_start "Count junit test files"
+function get_count_integration_files() {
+ start_end::group_start "Count integration test files"
local pattern_array=(
"^hadoop-ozone/dev-support/checks/_mvn_unit_report.sh"
"^hadoop-ozone/dev-support/checks/integration.sh"
- "^hadoop-ozone/dev-support/checks/unit.sh"
+ "^hadoop-ozone/integration-test"
+ "^hadoop-ozone/fault-injection-test/mini-chaos-tests"
"src/test/java"
- "src/test/resources"
+ )
+ local ignore_array=(
+ "^hadoop-hdds/.*/src/test/java/.*/Test.*.java"
+ "^hadoop-ozone/[a-eghj-z].*/src/test/java/.*/Test.*.java"
Review comment:
This regex is a bit puzzling, at first.
Perhaps a comment like:
#capture all directories except the "integration-test" directory
?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai merged pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
adoroszlai merged pull request #3061:
URL: https://github.com/apache/ozone/pull/3061
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GeorgeJahad commented on a change in pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
GeorgeJahad commented on a change in pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#discussion_r806277769
##########
File path: dev-support/ci/selective_ci_checks.sh
##########
@@ -242,18 +242,25 @@ function get_count_doc_files() {
start_end::group_end
}
-function get_count_junit_files() {
- start_end::group_start "Count junit test files"
+function get_count_integration_files() {
+ start_end::group_start "Count integration test files"
local pattern_array=(
"^hadoop-ozone/dev-support/checks/_mvn_unit_report.sh"
"^hadoop-ozone/dev-support/checks/integration.sh"
- "^hadoop-ozone/dev-support/checks/unit.sh"
+ "^hadoop-ozone/integration-test"
+ "^hadoop-ozone/fault-injection-test/mini-chaos-tests"
"src/test/java"
- "src/test/resources"
+ )
+ local ignore_array=(
+ "^hadoop-hdds/.*/src/test/java/.*/Test.*.java"
+ "^hadoop-ozone/[a-eghj-z].*/src/test/java/.*/Test.*.java"
Review comment:
looks good to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GeorgeJahad commented on a change in pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
GeorgeJahad commented on a change in pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#discussion_r806174800
##########
File path: dev-support/ci/selective_ci_checks.sh
##########
@@ -242,18 +242,25 @@ function get_count_doc_files() {
start_end::group_end
}
-function get_count_junit_files() {
- start_end::group_start "Count junit test files"
+function get_count_integration_files() {
+ start_end::group_start "Count integration test files"
local pattern_array=(
"^hadoop-ozone/dev-support/checks/_mvn_unit_report.sh"
"^hadoop-ozone/dev-support/checks/integration.sh"
- "^hadoop-ozone/dev-support/checks/unit.sh"
+ "^hadoop-ozone/integration-test"
+ "^hadoop-ozone/fault-injection-test/mini-chaos-tests"
"src/test/java"
- "src/test/resources"
+ )
+ local ignore_array=(
Review comment:
I recall you mentioned here:
https://github.com/apache/ozone/pull/2948#issuecomment-1022424382
some helper classes may need to be renamed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#discussion_r803420551
##########
File path: dev-support/ci/selective_ci_checks.sh
##########
@@ -399,19 +406,18 @@ function check_needs_unit_test() {
local pattern_array=(
"^hadoop-ozone/dev-support/checks/_mvn_unit_report.sh"
"^hadoop-ozone/dev-support/checks/unit.sh"
- "pom.xml"
Review comment:
With this patch unit test will be triggered via this branch for `pom.xml` changes:
https://github.com/apache/ozone/blob/e3378947c491dbf66a30c010ac339ddd2832daf9/dev-support/ci/selective_ci_checks.sh#L466-L473
This is covered by the "pom change" test case:
https://github.com/apache/ozone/blob/e3378947c491dbf66a30c010ac339ddd2832daf9/dev-support/ci/selective_ci_checks.bats#L201-L210
BTW same applies to main (non-test) Java changes. Now I've added a test case for that, too:
https://github.com/apache/ozone/blob/5843ab95c4f4a5c3eba3111b86da258f0f0ad499/dev-support/ci/selective_ci_checks.bats#L168-L177
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a change in pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#discussion_r803166219
##########
File path: dev-support/ci/selective_ci_checks.sh
##########
@@ -399,19 +406,18 @@ function check_needs_unit_test() {
local pattern_array=(
"^hadoop-ozone/dev-support/checks/_mvn_unit_report.sh"
"^hadoop-ozone/dev-support/checks/unit.sh"
- "pom.xml"
Review comment:
Does this mean changing the `pom.xml` won't trigger the unit test?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a change in pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#discussion_r804082817
##########
File path: dev-support/ci/selective_ci_checks.bats
##########
@@ -99,6 +99,39 @@ load bats-assert/load.bash
assert_output -p needs-kubernetes-tests=false
}
+@test "integration and unit: script change" {
+ run dev-support/ci/selective_ci_checks.sh c6850484f
Review comment:
Yup. From my limited understanding of the script, it looks like the SHA argument for `selective_ci_checks.sh` is first read here:
https://github.com/apache/ozone/blob/5843ab95c4f4a5c3eba3111b86da258f0f0ad499/dev-support/ci/selective_ci_checks.sh#L512-L526
after that it is seemingly only used here in `get_changed_files()`:
https://github.com/apache/ozone/blob/5843ab95c4f4a5c3eba3111b86da258f0f0ad499/dev-support/ci/selective_ci_checks.sh#L62-L70
where the script generate the diff list of changed files from the given commit by running `git diff-tree --no-commit-id --name-only -r SHA~1 SHA` command.
So in the case of c6850484f, `CHANGED_FILES=hadoop-ozone/dev-support/checks/_mvn_unit_report.sh` (or in the case of multiple file change commits like 86a771dfe, the command gives relative file paths to the root separated by **line feeds**).
Therefore, wouldn't that be more straight forward to feed the file paths (a.k.a. `CHANGED_FILES`) directly to the `get_changed_files()` function rather than feeding a SHA then generating the changed files list? I'm guessing there was a historical reason for it? e.g. some other functions are/were using the SHA for different purposes? But even then it is questionable if it is worth it to address this. This is more of a simple thought/question from me rather than a suggestion here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#discussion_r804113896
##########
File path: dev-support/ci/selective_ci_checks.bats
##########
@@ -99,6 +99,39 @@ load bats-assert/load.bash
assert_output -p needs-kubernetes-tests=false
}
+@test "integration and unit: script change" {
+ run dev-support/ci/selective_ci_checks.sh c6850484f
Review comment:
> [the SHA] is passed into here in `get_changed_files()`
> ...
> where the script generate the diff list of changed files from the given commit
vs.
> So wouldn't that be more straight forward to feed the file paths (a.k.a. `CHANGED_FILES`) directly to the `get_changed_files()` function rather than feeding a SHA?
The main task of `get_changed_files()` is generating the list of changed files. (It also outputs it and handles the degenerate case of no changed files.)
So if we passed the list of changed files to this function, what would be its remaining job? We'd also need some other function or code block to calculate the list of changed files, as the script's input is a SHA (normally the PR merge commit).
I think it's better to exercise it end-to-end by passing SHAs in the test, too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#issuecomment-1039900852
Thanks @GeorgeJahad and @smengcl for the review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a change in pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#discussion_r803163461
##########
File path: dev-support/ci/selective_ci_checks.bats
##########
@@ -99,6 +99,39 @@ load bats-assert/load.bash
assert_output -p needs-kubernetes-tests=false
}
+@test "integration and unit: script change" {
+ run dev-support/ci/selective_ci_checks.sh c6850484f
Review comment:
Interesting. Thanks for the explanation.
Sounds a bit like a "hack" to me, but acceptable.
A cleaner approach would be to, in some way, simply pass the relative file path to the project root to the `get_changed_files()` function in `selective_ci_checks.sh`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a change in pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#discussion_r803166219
##########
File path: dev-support/ci/selective_ci_checks.sh
##########
@@ -399,19 +406,18 @@ function check_needs_unit_test() {
local pattern_array=(
"^hadoop-ozone/dev-support/checks/_mvn_unit_report.sh"
"^hadoop-ozone/dev-support/checks/unit.sh"
- "pom.xml"
Review comment:
Does the removal of this line mean changing the `pom.xml` won't trigger the unit test?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a change in pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#discussion_r803155568
##########
File path: dev-support/ci/selective_ci_checks.bats
##########
@@ -99,6 +99,39 @@ load bats-assert/load.bash
assert_output -p needs-kubernetes-tests=false
}
+@test "integration and unit: script change" {
+ run dev-support/ci/selective_ci_checks.sh c6850484f
Review comment:
Could you help me understand how this specific SHA value is chosen here?
It seems this specific SHA [c6850484f](https://github.com/apache/ozone/commits/c6850484f) dates back to Sep 2019?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a change in pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#discussion_r803155568
##########
File path: dev-support/ci/selective_ci_checks.bats
##########
@@ -99,6 +99,39 @@ load bats-assert/load.bash
assert_output -p needs-kubernetes-tests=false
}
+@test "integration and unit: script change" {
+ run dev-support/ci/selective_ci_checks.sh c6850484f
Review comment:
Could you help me understand why this specific SHA value chosen here?
It seems this specific SHA [c6850484f](https://github.com/apache/ozone/commits/c6850484f) dates back to Sep 2019?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a change in pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#discussion_r804150246
##########
File path: dev-support/ci/selective_ci_checks.bats
##########
@@ -99,6 +99,39 @@ load bats-assert/load.bash
assert_output -p needs-kubernetes-tests=false
}
+@test "integration and unit: script change" {
+ run dev-support/ci/selective_ci_checks.sh c6850484f
Review comment:
> So if we passed the list of changed files to this function, what would be its remaining job?
Yes you are right. I should have said "feed the file paths directly to the script" rather than saying "feed the file paths to `get_changed_files()`".
Is `get_changed_files()` called anywhere else? I can only find it called inside the script itself so far.
> I think it's better to exercise it end-to-end by passing SHAs in the test, too.
I'm ok with this. I don't have a strong opinion. The discussion is merely out of my curiosity (and probably my lack of further understanding as well at this stage). I'm sure there is a good reason for it. -- And yes it looks clean when we only need to pass in a SHA instead of a long list of changed files.
Anyway I am +1 on the patch itself. Thanks again for improving the run times!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#discussion_r804423872
##########
File path: dev-support/ci/selective_ci_checks.bats
##########
@@ -99,6 +99,39 @@ load bats-assert/load.bash
assert_output -p needs-kubernetes-tests=false
}
+@test "integration and unit: script change" {
+ run dev-support/ci/selective_ci_checks.sh c6850484f
Review comment:
> I should have said "feed the file paths directly to the script"
Yep, I was kind of guessing that.
SHA is available in Github Actions as a context variable, but list of changed files is not. So we need the script to calculate that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai merged pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
adoroszlai merged pull request #3061:
URL: https://github.com/apache/ozone/pull/3061
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a change in pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#discussion_r804082817
##########
File path: dev-support/ci/selective_ci_checks.bats
##########
@@ -99,6 +99,39 @@ load bats-assert/load.bash
assert_output -p needs-kubernetes-tests=false
}
+@test "integration and unit: script change" {
+ run dev-support/ci/selective_ci_checks.sh c6850484f
Review comment:
Yup. From my limited understanding of the script, it looks like the SHA argument for `selective_ci_checks.sh` is first read here:
https://github.com/apache/ozone/blob/5843ab95c4f4a5c3eba3111b86da258f0f0ad499/dev-support/ci/selective_ci_checks.sh#L512-L526
after that it is seemingly only used here in `get_changed_files()`:
https://github.com/apache/ozone/blob/5843ab95c4f4a5c3eba3111b86da258f0f0ad499/dev-support/ci/selective_ci_checks.sh#L62-L70
where the script generate the diff list of changed files from the given commit by running `git diff-tree --no-commit-id --name-only -r SHA~1 SHA` command.
So in the case of c6850484f, `CHANGED_FILES=hadoop-ozone/dev-support/checks/_mvn_unit_report.sh` (or in the case of multiple file change commits like 86a771dfe, the command gives relative file paths to the root separated by **line feeds**).
Therefore, wouldn't that be more straight forward to feed the file paths (a.k.a. `CHANGED_FILES`) directly to the `get_changed_files()` function rather than feeding a SHA? I'm guessing there was a historical reason for it? Some other functions are/were using the SHA for different purposes? But even then it is questionable if it is worth it to address this. This is more of a simple thought/question from me rather than a suggestion here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a change in pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#discussion_r804082817
##########
File path: dev-support/ci/selective_ci_checks.bats
##########
@@ -99,6 +99,39 @@ load bats-assert/load.bash
assert_output -p needs-kubernetes-tests=false
}
+@test "integration and unit: script change" {
+ run dev-support/ci/selective_ci_checks.sh c6850484f
Review comment:
Yup. From my limited understanding of the script, it looks like the SHA argument for `selective_ci_checks.sh` is first read here:
https://github.com/apache/ozone/blob/5843ab95c4f4a5c3eba3111b86da258f0f0ad499/dev-support/ci/selective_ci_checks.sh#L512-L526
after that it is usedhere in `get_changed_files()`:
https://github.com/apache/ozone/blob/5843ab95c4f4a5c3eba3111b86da258f0f0ad499/dev-support/ci/selective_ci_checks.sh#L62-L70
where the script generate the diff list of changed files from the given commit by running `git diff-tree --no-commit-id --name-only -r SHA~1 SHA` command.
So in the case of c6850484f, `CHANGED_FILES=hadoop-ozone/dev-support/checks/_mvn_unit_report.sh` (or in the case of multiple file change commits like 86a771dfe, the command gives relative file paths to the root separated by **line feeds**).
Therefore, wouldn't that be more straight forward to feed the file paths (a.k.a. `CHANGED_FILES`) directly to the `get_changed_files()` function rather than feeding a SHA? I'm guessing there was a historical reason for it? Some other functions are/were using the SHA for different purposes? But even then it is questionable if it is worth it to address this. This is more of a simple thought/question from me rather than a suggestion here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GeorgeJahad commented on a change in pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
GeorgeJahad commented on a change in pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#discussion_r806173443
##########
File path: dev-support/ci/selective_ci_checks.sh
##########
@@ -242,18 +242,25 @@ function get_count_doc_files() {
start_end::group_end
}
-function get_count_junit_files() {
- start_end::group_start "Count junit test files"
+function get_count_integration_files() {
+ start_end::group_start "Count integration test files"
local pattern_array=(
"^hadoop-ozone/dev-support/checks/_mvn_unit_report.sh"
"^hadoop-ozone/dev-support/checks/integration.sh"
- "^hadoop-ozone/dev-support/checks/unit.sh"
+ "^hadoop-ozone/integration-test"
+ "^hadoop-ozone/fault-injection-test/mini-chaos-tests"
"src/test/java"
- "src/test/resources"
+ )
+ local ignore_array=(
+ "^hadoop-hdds/.*/src/test/java/.*/Test.*.java"
+ "^hadoop-ozone/[a-eghj-z].*/src/test/java/.*/Test.*.java"
Review comment:
This regex is a bit puzzling, at first.
Perhaps a comment like:
#capture all directories except the "integration-test" directory
?
##########
File path: dev-support/ci/selective_ci_checks.sh
##########
@@ -242,18 +242,25 @@ function get_count_doc_files() {
start_end::group_end
}
-function get_count_junit_files() {
- start_end::group_start "Count junit test files"
+function get_count_integration_files() {
+ start_end::group_start "Count integration test files"
local pattern_array=(
"^hadoop-ozone/dev-support/checks/_mvn_unit_report.sh"
"^hadoop-ozone/dev-support/checks/integration.sh"
- "^hadoop-ozone/dev-support/checks/unit.sh"
+ "^hadoop-ozone/integration-test"
+ "^hadoop-ozone/fault-injection-test/mini-chaos-tests"
"src/test/java"
- "src/test/resources"
+ )
+ local ignore_array=(
Review comment:
I recall you mentioned here:
https://github.com/apache/ozone/pull/2948#issuecomment-1022424382
some helper classes may need to be renamed?
##########
File path: dev-support/ci/selective_ci_checks.sh
##########
@@ -242,18 +242,25 @@ function get_count_doc_files() {
start_end::group_end
}
-function get_count_junit_files() {
- start_end::group_start "Count junit test files"
+function get_count_integration_files() {
+ start_end::group_start "Count integration test files"
local pattern_array=(
"^hadoop-ozone/dev-support/checks/_mvn_unit_report.sh"
"^hadoop-ozone/dev-support/checks/integration.sh"
- "^hadoop-ozone/dev-support/checks/unit.sh"
+ "^hadoop-ozone/integration-test"
+ "^hadoop-ozone/fault-injection-test/mini-chaos-tests"
"src/test/java"
- "src/test/resources"
+ )
+ local ignore_array=(
+ "^hadoop-hdds/.*/src/test/java/.*/Test.*.java"
+ "^hadoop-ozone/[a-eghj-z].*/src/test/java/.*/Test.*.java"
Review comment:
looks good to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#issuecomment-1039900852
Thanks @GeorgeJahad and @smengcl for the review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#issuecomment-1033729143
@GeorgeJahad @smengcl Here's my last selective check improvement for some time :), skipping integration tests for unit test changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a change in pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#discussion_r803166219
##########
File path: dev-support/ci/selective_ci_checks.sh
##########
@@ -399,19 +406,18 @@ function check_needs_unit_test() {
local pattern_array=(
"^hadoop-ozone/dev-support/checks/_mvn_unit_report.sh"
"^hadoop-ozone/dev-support/checks/unit.sh"
- "pom.xml"
Review comment:
Does the removal of this line mean changing the project root `pom.xml` won't trigger the unit test?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a change in pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#discussion_r804069901
##########
File path: dev-support/ci/selective_ci_checks.sh
##########
@@ -399,19 +406,18 @@ function check_needs_unit_test() {
local pattern_array=(
"^hadoop-ozone/dev-support/checks/_mvn_unit_report.sh"
"^hadoop-ozone/dev-support/checks/unit.sh"
- "pom.xml"
Review comment:
Got it! Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GeorgeJahad commented on pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
GeorgeJahad commented on pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#issuecomment-1039617752
+1 on the pr.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#discussion_r803405281
##########
File path: dev-support/ci/selective_ci_checks.bats
##########
@@ -99,6 +99,39 @@ load bats-assert/load.bash
assert_output -p needs-kubernetes-tests=false
}
+@test "integration and unit: script change" {
+ run dev-support/ci/selective_ci_checks.sh c6850484f
Review comment:
Sorry, I'm not sure I understand the proposed cleaner approach. Can you please clarify?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a change in pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#discussion_r804150246
##########
File path: dev-support/ci/selective_ci_checks.bats
##########
@@ -99,6 +99,39 @@ load bats-assert/load.bash
assert_output -p needs-kubernetes-tests=false
}
+@test "integration and unit: script change" {
+ run dev-support/ci/selective_ci_checks.sh c6850484f
Review comment:
> So if we passed the list of changed files to this function, what would be its remaining job?
Yes you are right. I should have said "feed the file paths directly to the script" rather than saying "feed the file paths to `get_changed_files()`".
Is `get_changed_files()` called anywhere else? I can only find it called inside the script itself so far.
> I think it's better to exercise it end-to-end by passing SHAs in the test, too.
I'm ok with this. I don't have a strong opinion. The discussion is merely out of my curiosity (and probably my lack of further understanding as well at this stage). I'm sure there is a good reason for it.
Anyway I am +1 on the patch itself. Thanks again for improving the run times!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a change in pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#discussion_r804082817
##########
File path: dev-support/ci/selective_ci_checks.bats
##########
@@ -99,6 +99,39 @@ load bats-assert/load.bash
assert_output -p needs-kubernetes-tests=false
}
+@test "integration and unit: script change" {
+ run dev-support/ci/selective_ci_checks.sh c6850484f
Review comment:
Yup. From my limited understanding of the script, it looks like the SHA argument for `selective_ci_checks.sh` is first read here:
https://github.com/apache/ozone/blob/5843ab95c4f4a5c3eba3111b86da258f0f0ad499/dev-support/ci/selective_ci_checks.sh#L512-L526
after that it is seemingly only used here in `get_changed_files()`:
https://github.com/apache/ozone/blob/5843ab95c4f4a5c3eba3111b86da258f0f0ad499/dev-support/ci/selective_ci_checks.sh#L62-L70
where the script generate the diff list of changed files from the given commit by running `git diff-tree --no-commit-id --name-only -r SHA~1 SHA` command.
So in the case of c6850484f, `CHANGED_FILES=hadoop-ozone/dev-support/checks/_mvn_unit_report.sh` (or in the case of multiple file change commits like 86a771dfe, the command gives relative file paths to the root separated by **line feeds**).
Therefore, wouldn't that be more straight forward to feed the file paths (a.k.a. `CHANGED_FILES`) directly to the `get_changed_files()` function rather than feeding a SHA then generating the changed files list? I'm guessing there was a historical reason for it? e.g. some other functions are/were using the SHA for different purposes? But even then it is questionable if it is worth it to address this. This is more of a simple thought/question from me rather than a suggestion here. (Even if this "suggestion" might be deemed as worth addressing it isn't related to this PR itself.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a change in pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#discussion_r804082817
##########
File path: dev-support/ci/selective_ci_checks.bats
##########
@@ -99,6 +99,39 @@ load bats-assert/load.bash
assert_output -p needs-kubernetes-tests=false
}
+@test "integration and unit: script change" {
+ run dev-support/ci/selective_ci_checks.sh c6850484f
Review comment:
Yup. From my limited understanding of the script, it looks like the SHA argument for `selective_ci_checks.sh` is first read here:
https://github.com/apache/ozone/blob/5843ab95c4f4a5c3eba3111b86da258f0f0ad499/dev-support/ci/selective_ci_checks.sh#L512-L526
after that it is used here in `get_changed_files()`:
https://github.com/apache/ozone/blob/5843ab95c4f4a5c3eba3111b86da258f0f0ad499/dev-support/ci/selective_ci_checks.sh#L62-L70
where the script generate the diff list of changed files from the given commit by running `git diff-tree --no-commit-id --name-only -r SHA~1 SHA` command.
So in the case of c6850484f, `CHANGED_FILES=hadoop-ozone/dev-support/checks/_mvn_unit_report.sh` (or in the case of multiple file change commits like 86a771dfe, the command gives relative file paths to the root separated by **line feeds**).
Therefore, wouldn't that be more straight forward to feed the file paths (a.k.a. `CHANGED_FILES`) directly to the `get_changed_files()` function rather than feeding a SHA? I'm guessing there was a historical reason for it? Some other functions are/were using the SHA for different purposes? But even then it is questionable if it is worth it to address this. This is more of a simple thought/question from me rather than a suggestion here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#discussion_r803158622
##########
File path: dev-support/ci/selective_ci_checks.bats
##########
@@ -99,6 +99,39 @@ load bats-assert/load.bash
assert_output -p needs-kubernetes-tests=false
}
+@test "integration and unit: script change" {
+ run dev-support/ci/selective_ci_checks.sh c6850484f
Review comment:
Sure. I looked for a commit that only changed `_mvn_unit_report.sh`. This file is shared between _unit_ and _integration_ checks, so it should trigger both. It also triggers _bats_ due to change in a shell script, although there is no bats test for this specific script. And _rat_ is needed for all changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GeorgeJahad commented on pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
GeorgeJahad commented on pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#issuecomment-1039617752
+1 on the pr.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#discussion_r806194367
##########
File path: dev-support/ci/selective_ci_checks.sh
##########
@@ -242,18 +242,25 @@ function get_count_doc_files() {
start_end::group_end
}
-function get_count_junit_files() {
- start_end::group_start "Count junit test files"
+function get_count_integration_files() {
+ start_end::group_start "Count integration test files"
local pattern_array=(
"^hadoop-ozone/dev-support/checks/_mvn_unit_report.sh"
"^hadoop-ozone/dev-support/checks/integration.sh"
- "^hadoop-ozone/dev-support/checks/unit.sh"
+ "^hadoop-ozone/integration-test"
+ "^hadoop-ozone/fault-injection-test/mini-chaos-tests"
"src/test/java"
- "src/test/resources"
+ )
+ local ignore_array=(
Review comment:
I have renamed the ones I found in HDDS-6227 (11605e171).
##########
File path: dev-support/ci/selective_ci_checks.sh
##########
@@ -242,18 +242,25 @@ function get_count_doc_files() {
start_end::group_end
}
-function get_count_junit_files() {
- start_end::group_start "Count junit test files"
+function get_count_integration_files() {
+ start_end::group_start "Count integration test files"
local pattern_array=(
"^hadoop-ozone/dev-support/checks/_mvn_unit_report.sh"
"^hadoop-ozone/dev-support/checks/integration.sh"
- "^hadoop-ozone/dev-support/checks/unit.sh"
+ "^hadoop-ozone/integration-test"
+ "^hadoop-ozone/fault-injection-test/mini-chaos-tests"
"src/test/java"
- "src/test/resources"
+ )
+ local ignore_array=(
+ "^hadoop-hdds/.*/src/test/java/.*/Test.*.java"
+ "^hadoop-ozone/[a-eghj-z].*/src/test/java/.*/Test.*.java"
Review comment:
Thanks @GeorgeJahad for the reminder on adding comment.
How about this?
```
# Ozone's unit test naming convention: Test*.java
# The following makes this filter ignore all tests except those in
# integration-test and fault-injection-test.
# Directories starting with `i` under hadoop-ozone need to be listed
# explicitly, other subdirectories are captured by the second item.
local ignore_array=(
"^hadoop-hdds/.*/src/test/java/.*/Test.*.java"
"^hadoop-ozone/[a-eghj-z].*/src/test/java/.*/Test.*.java"
"^hadoop-ozone/insight/src/test/java/.*/Test.*.java"
"^hadoop-ozone/interface-client/src/test/java/.*/Test.*.java"
"^hadoop-ozone/interface-storage/src/test/java/.*/Test.*.java"
)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a change in pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#discussion_r804082817
##########
File path: dev-support/ci/selective_ci_checks.bats
##########
@@ -99,6 +99,39 @@ load bats-assert/load.bash
assert_output -p needs-kubernetes-tests=false
}
+@test "integration and unit: script change" {
+ run dev-support/ci/selective_ci_checks.sh c6850484f
Review comment:
Yup. From my limited understanding of the script, it looks like the hash argument for `selective_ci_checks.sh` is only used here:
https://github.com/apache/ozone/blob/5843ab95/dev-support/ci/selective_ci_checks.sh#L512-L526
after that it is passed into here in `get_changed_files()`:
https://github.com/apache/ozone/blob/5843ab95/dev-support/ci/selective_ci_checks.sh#L62-L70
where the script generate the diff list of changed files from the given commit by running `git diff-tree --no-commit-id --name-only -r SHA~1 SHA` command.
So in the case of c6850484f, `CHANGED_FILES=hadoop-ozone/dev-support/checks/_mvn_unit_report.sh` (or in the case of multiple file change commits like 86a771dfe, the command gives relative file paths to the root separated by **line feeds**).
So wouldn't that be more straight forward to feed the file paths (a.k.a. `CHANGED_FILES`) directly to the `get_changed_files()` function rather than feeding a SHA? I'm guessing there was a historical reason for it? Some other functions are/were using the SHA for different purposes? But even then it is questionable if it is worth it to address this. This is more of a simple thought/question from me rather than a suggestion here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#discussion_r803158622
##########
File path: dev-support/ci/selective_ci_checks.bats
##########
@@ -99,6 +99,39 @@ load bats-assert/load.bash
assert_output -p needs-kubernetes-tests=false
}
+@test "integration and unit: script change" {
+ run dev-support/ci/selective_ci_checks.sh c6850484f
Review comment:
Sure. I looked for a commit that only changed `_mvn_unit_report.sh`. This file is shared between _unit_ and _integration_ checks, so it should trigger both. It also triggers _bats_ due to change in a shell script, although there is no bats test for this specific script. And _rat_ is needed for all changes.
Date does not really matter. Started looking on both ends of history.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a change in pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#discussion_r804082817
##########
File path: dev-support/ci/selective_ci_checks.bats
##########
@@ -99,6 +99,39 @@ load bats-assert/load.bash
assert_output -p needs-kubernetes-tests=false
}
+@test "integration and unit: script change" {
+ run dev-support/ci/selective_ci_checks.sh c6850484f
Review comment:
Yup. From my limited understanding of the script, it looks like the hash argument for `selective_ci_checks.sh` is only used here:
https://github.com/apache/ozone/blob/5843ab95c4f4a5c3eba3111b86da258f0f0ad499/dev-support/ci/selective_ci_checks.sh#L512-L526
after that it is passed into here in `get_changed_files()`:
https://github.com/apache/ozone/blob/5843ab95c4f4a5c3eba3111b86da258f0f0ad499/dev-support/ci/selective_ci_checks.sh#L62-L70
where the script generate the diff list of changed files from the given commit by running `git diff-tree --no-commit-id --name-only -r SHA~1 SHA` command.
So in the case of c6850484f, `CHANGED_FILES=hadoop-ozone/dev-support/checks/_mvn_unit_report.sh` (or in the case of multiple file change commits like 86a771dfe, the command gives relative file paths to the root separated by **line feeds**).
So wouldn't that be more straight forward to feed the file paths (a.k.a. `CHANGED_FILES`) directly to the `get_changed_files()` function rather than feeding a SHA? I'm guessing there was a historical reason for it? Some other functions are/were using the SHA for different purposes? But even then it is questionable if it is worth it to address this. This is more of a simple thought/question from me rather than a suggestion here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#discussion_r806194367
##########
File path: dev-support/ci/selective_ci_checks.sh
##########
@@ -242,18 +242,25 @@ function get_count_doc_files() {
start_end::group_end
}
-function get_count_junit_files() {
- start_end::group_start "Count junit test files"
+function get_count_integration_files() {
+ start_end::group_start "Count integration test files"
local pattern_array=(
"^hadoop-ozone/dev-support/checks/_mvn_unit_report.sh"
"^hadoop-ozone/dev-support/checks/integration.sh"
- "^hadoop-ozone/dev-support/checks/unit.sh"
+ "^hadoop-ozone/integration-test"
+ "^hadoop-ozone/fault-injection-test/mini-chaos-tests"
"src/test/java"
- "src/test/resources"
+ )
+ local ignore_array=(
Review comment:
I have renamed the ones I found in HDDS-6227 (11605e171).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #3061: HDDS-6186. Selective checks: skip integration check for unit test changes
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #3061:
URL: https://github.com/apache/ozone/pull/3061#discussion_r806200032
##########
File path: dev-support/ci/selective_ci_checks.sh
##########
@@ -242,18 +242,25 @@ function get_count_doc_files() {
start_end::group_end
}
-function get_count_junit_files() {
- start_end::group_start "Count junit test files"
+function get_count_integration_files() {
+ start_end::group_start "Count integration test files"
local pattern_array=(
"^hadoop-ozone/dev-support/checks/_mvn_unit_report.sh"
"^hadoop-ozone/dev-support/checks/integration.sh"
- "^hadoop-ozone/dev-support/checks/unit.sh"
+ "^hadoop-ozone/integration-test"
+ "^hadoop-ozone/fault-injection-test/mini-chaos-tests"
"src/test/java"
- "src/test/resources"
+ )
+ local ignore_array=(
+ "^hadoop-hdds/.*/src/test/java/.*/Test.*.java"
+ "^hadoop-ozone/[a-eghj-z].*/src/test/java/.*/Test.*.java"
Review comment:
Thanks @GeorgeJahad for the reminder on adding comment.
How about this?
```
# Ozone's unit test naming convention: Test*.java
# The following makes this filter ignore all tests except those in
# integration-test and fault-injection-test.
# Directories starting with `i` under hadoop-ozone need to be listed
# explicitly, other subdirectories are captured by the second item.
local ignore_array=(
"^hadoop-hdds/.*/src/test/java/.*/Test.*.java"
"^hadoop-ozone/[a-eghj-z].*/src/test/java/.*/Test.*.java"
"^hadoop-ozone/insight/src/test/java/.*/Test.*.java"
"^hadoop-ozone/interface-client/src/test/java/.*/Test.*.java"
"^hadoop-ozone/interface-storage/src/test/java/.*/Test.*.java"
)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org