You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/09/27 19:47:57 UTC
[GitHub] [airflow] OmairK opened a new pull request #11179: In container bash tests via bats
OmairK opened a new pull request #11179:
URL: https://github.com/apache/airflow/pull/11179
Some of the bash tests needed the airflow ci container to be tested out so as part of #10576, this commit introduces "in container" bats test along with a pre-commit hook for easy integration.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11179: In container bash tests via bats
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11179:
URL: https://github.com/apache/airflow/pull/11179#discussion_r496496216
##########
File path: .pre-commit-config.yaml
##########
@@ -406,8 +412,9 @@ repos:
name: Run BATS bash tests for changed bash files
language: system
entry: "./scripts/ci/pre_commit/pre_commit_bat_tests.sh"
- files: ^breeze$|^breeze-complete$|\.sh$|\.bash$|.bats$
- pass_filenames: false
+ files: ^breeze$|^breeze-complete$|\.sh$|\.bash$|\.bats$
+ exclude: ^tests/bats/in_container/.*bats$
+ pass_filenames: true
Review comment:
See the general comment - I love that we can run the scripts individually and pass .bat files as parameters, but in pre-commit we should set pass-filenames to "false" because we always want to run all .bat tests if any of the relevant files change.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #11179: In container bash tests via bats
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #11179:
URL: https://github.com/apache/airflow/pull/11179#issuecomment-699935556
BATS failed, can you take a look at it please @OmairK
```
Run BATS bash tests for changed bash files........................................................................................Failed
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] OmairK commented on a change in pull request #11179: In container bash tests via bats
Posted by GitBox <gi...@apache.org>.
OmairK commented on a change in pull request #11179:
URL: https://github.com/apache/airflow/pull/11179#discussion_r495618175
##########
File path: tests/bats/in_container/bats_utils.bash
##########
@@ -0,0 +1,34 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+AIRFLOW_SOURCES=$(pwd)
+export AIRFLOW_SOURCES
+readonly AIRFLOW_SOURCES
+
+export DOCKER_BINARY_PATH=${AIRFLOW_SOURCES}/tests/bats/mock/docker.sh
Review comment:
Thanks, fixed it 35c2be2
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11179: In container bash tests via bats
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11179:
URL: https://github.com/apache/airflow/pull/11179#discussion_r495617084
##########
File path: tests/bats/in_container/test_in_container.bats
##########
@@ -0,0 +1,30 @@
+#!/usr/bin/env bats
+
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+setup() {
+ load bats_utils
Review comment:
As explaine above- I think we can remove those for now.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] OmairK commented on pull request #11179: In container bash tests via bats
Posted by GitBox <gi...@apache.org>.
OmairK commented on pull request #11179:
URL: https://github.com/apache/airflow/pull/11179#issuecomment-699680461
cc:@potiuk, @kaxil
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11179: In container bash tests via bats
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11179:
URL: https://github.com/apache/airflow/pull/11179#discussion_r496496600
##########
File path: .pre-commit-config.yaml
##########
@@ -381,6 +381,12 @@ repos:
pass_filenames: false
require_serial: true
additional_dependencies: ['pyyaml']
+ - id: bats-in-container-tests
+ name: Run in container bats tests
+ language: system
+ entry: "./scripts/ci/pre_commit/pre_commit_in_container_bats_test.sh"
+ files: ^tests/bats/in_container/.*.bats$
+ require_serial: true
Review comment:
Here all pass_filenames should be set to false/
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #11179: In container bash tests via bats
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #11179:
URL: https://github.com/apache/airflow/pull/11179#issuecomment-700599763
> I was thinking of adding another pre-commit hook to look for relevant bats file for a .sh, but we will need a consistent naming scheme for that to happen <test_relevant_file.bats>. Do you think this is something that needs to be there or am I having too much fun with bash and regex and should leave it for now .
I think this is a good idea, to do in the near future. Bash Unit tests are not the fastest ones and as we add more of those, pre-commit will take more and more time. So having 1-1 relationship: script/bats test is a good idea! And not very difficult one.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #11179: In container bash tests via bats
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #11179:
URL: https://github.com/apache/airflow/pull/11179#issuecomment-700521688
@OmairK. I think this is very close :). I already had similar cases in the past.
The scripts you have now will handle both cases very well - pre-commit run and manual execution :). But we need one more small change - change pass_filenames to false :).
Pre-commit works best if it acts on the files that got changed, but this case is a bit different. I think eventually we want to run all the relevant .bat tests any time any of the relevant .sh or .bat tests change (because we have no direct relation .sh -> .bat). On the other hand - it's great to have the scripts handy to manually run individual bats tests by providing them as parameters. And I think your scripts are great for that.
There should be two cases when pre-commits are run:
a) If any of the "out-container" .sh files or "out-container" .bat files change -> run all "out-container" bat tests
b) If any of the "in-container" .sh files or "in-container" .bat files change -> run all "in-container" bat tests
In the pre-commit there is 'pass_filename: false" option - which will simply run pre-commit without passing any of the filenames to script.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #11179: In container bash tests via bats
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #11179:
URL: https://github.com/apache/airflow/pull/11179
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] OmairK commented on pull request #11179: In container bash tests via bats
Posted by GitBox <gi...@apache.org>.
OmairK commented on pull request #11179:
URL: https://github.com/apache/airflow/pull/11179#issuecomment-700580202
> Pre-commit works best if it acts on the files that got changed, but this case is a bit different. I think eventually we want to run all the relevant .bat tests any time any of the relevant .sh or .bat tests change (because we have no direct relation .sh -> .bat).
I was thinking of adding another pre-commit hook to look for relevant bats file for a .sh, but we will need a consistent naming scheme for that to happen <test_relevant_file.bats>. Do you think this is something that needs to be there or am I having too much fun with bash and regex and should leave it for now :smile:.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11179: In container bash tests via bats
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11179:
URL: https://github.com/apache/airflow/pull/11179#discussion_r496497358
##########
File path: .pre-commit-config.yaml
##########
@@ -381,6 +381,12 @@ repos:
pass_filenames: false
require_serial: true
additional_dependencies: ['pyyaml']
+ - id: bats-in-container-tests
+ name: Run in container bats tests
+ language: system
+ entry: "./scripts/ci/pre_commit/pre_commit_in_container_bats_test.sh"
+ files: ^tests/bats/in_container/.*.bats$
Review comment:
We should change it to match also .sh files in the "in_container"
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11179: In container bash tests via bats
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11179:
URL: https://github.com/apache/airflow/pull/11179#discussion_r495616780
##########
File path: tests/bats/in_container/bats_utils.bash
##########
@@ -0,0 +1,34 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+AIRFLOW_SOURCES=$(pwd)
+export AIRFLOW_SOURCES
+readonly AIRFLOW_SOURCES
+
+export DOCKER_BINARY_PATH=${AIRFLOW_SOURCES}/tests/bats/mock/docker.sh
Review comment:
All those lines are not really needed. They are needed for Host scripts (in the host scripts we are loading _all_libs.sh and we are executing al the kubectl/kind etc .- but inside the container those are not really needed. I think the whole load bat_utls. step is not needed in fact. Maybe we will come up with some common utils to load over time but I think no need for this now.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] OmairK commented on pull request #11179: In container bash tests via bats
Posted by GitBox <gi...@apache.org>.
OmairK commented on pull request #11179:
URL: https://github.com/apache/airflow/pull/11179#issuecomment-700197993
> BATS failed, can you take a look at it please @OmairK
>
> ```
> Run BATS bash tests for changed bash files........................................................................................Failed
> ```
Yes, I looked into it the bats-tests pre-commit hook its running bats on .sh files rather than just .bats scripts.
```
- id: bats-tests
name: Run BATS bash tests for changed bash files
language: system
entry: "./scripts/ci/pre_commit/pre_commit_bat_tests.sh"
files: ^breeze$|^breeze-complete$|\.sh$|\.bash$|.bats
exclude: ^tests/bats/in_container
```
I am thinking of modifying this pre-commit hook to run:
- Only the bats tests that were created/changed in case of only .bats files being added/updated.
- All bats tests (excluding the in_container) if the change has a mix of shell scripts and bats tests.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11179: In container bash tests via bats
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11179:
URL: https://github.com/apache/airflow/pull/11179#discussion_r496498168
##########
File path: .pre-commit-config.yaml
##########
@@ -406,8 +412,9 @@ repos:
name: Run BATS bash tests for changed bash files
language: system
entry: "./scripts/ci/pre_commit/pre_commit_bat_tests.sh"
- files: ^breeze$|^breeze-complete$|\.sh$|\.bash$|.bats$
- pass_filenames: false
+ files: ^breeze$|^breeze-complete$|\.sh$|\.bash$|\.bats$
+ exclude: ^tests/bats/in_container/.*bats$
Review comment:
We should also exclude all the .sh files which are in_container.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org