code review changes

Signed-off-by: nadav mizrahi <[email protected]>

Author: nadavMiz

docs/dev_guide/ceph_s3_tests/ceph_s3_tests_guide.md

@@ -6,7 +6,8 @@
      2) Run All Ceph S3 Tests
      3) Run a Single Ceph S3 Test
      4) Debug a Single Ceph S3 Test
-     5) Examples
+     5) Compare to AWS Response (Inside Tester Pod)
+     6) Examples
 * This guide describes developer steps to run Ceph S3 on a noobaa system on minikube.
 
 ## General Settings For Ceph S3 Tests
@@ -90,6 +91,12 @@ kubectl logs job/noobaa-tests-s3 -f
 
 We run all the tests except the tests that appear in the lists `src/test/system_tests/ceph_s3_tests/s3-tests-lists` if you would like to add or remove a test you can edit those files (and then repeat the steps starting from 'Build Core And Tester Images (Noobaa-Core)' above).
 
+Some tests are marked to be skipped in the tests code. usually because the enviorment doesn't support certain criteria. those tests will appear with a `[WARN]` tag and will be marked as "Test Skipped", for example:
+`[WARN] CONSOLE:: Test skipped: s3tests_boto3/functional/test_s3.py::test_lifecycle_transition`
+
+In the test code the function:
+`pytest.skip("")` will mark them to be skipped.
+
 ## Run a Single Ceph S3 Test
 
 ### 1) Prerequisites:
@@ -137,20 +144,26 @@ Run the script that will create the necessary accounts in noobaa and update the
 node ./src/test/system_tests/ceph_s3_tests/test_ceph_s3_config_setup.js
 ```
 
-Note: If you want to ignore PythonDeprecationWarnings use (which will then ignore all Python warnings, so keep that in mind):
-```bash
-export PYTHONWARNINGS="ignore"
-```
+Note: If you want to ignore PythonDeprecationWarnings add the following flag to the test command (which will then ignore all Python warnings, so keep that in mind):
+`-- --disable-pytest-warnings`
+
+note that every flag that comes after `--` is past to pytest from tox. so if there is already `--` in the command just put `--disable-pytest-warnings` as part of the flags after it, no need to add another `--` notation.
+
+for example to add --disable-pytest-warnings to the command:
+`S3TEST_CONF=${PWD}/src/test/system_tests/ceph_s3_tests/test_ceph_s3_config.conf tox -c src/test/system_tests/ceph_s3_tests/s3-tests/tox.ini -- -m 'not fails_on_aws' ${PWD}/src/test/system_tests/ceph_s3_tests/s3-tests/s3tests_boto3/functional/test_s3.py::test_account_usage`
+
+it should be:
+`S3TEST_CONF=${PWD}/src/test/system_tests/ceph_s3_tests/test_ceph_s3_config.conf tox -c src/test/system_tests/ceph_s3_tests/s3-tests/tox.ini -- -m 'not fails_on_aws --disable-pytest-warnings ${PWD}/src/test/system_tests/ceph_s3_tests/s3-tests/s3tests_boto3/functional/test_s3.py::test_account_usage`
 
 ### 5) Run a Test (Inside The Tester Pod)
 To run a test, from noobaa working directory:
 ```bash
-S3TEST_CONF=$PWD/src/test/system_tests/ceph_s3_tests/test_ceph_s3_config.conf tox -c src/test/system_tests/ceph_s3_tests/s3_tests/tox.ini $PWD/src/test/system_tests/ceph_s3_tests/s3-tests/<test_name>
+S3TEST_CONF=${PWD}/src/test/system_tests/ceph_s3_tests/test_ceph_s3_config.conf tox -c src/test/system_tests/ceph_s3_tests/s3-tests/tox.ini ${PWD}/src/test/system_tests/ceph_s3_tests/s3-tests/<test_name>
 ```
 This should run the test on the noobaa deployment we've set up.
 
 #### Test Name
-You can find a list of tests in the doc inside the file `ceph_s3_tests_list_single_test.txt`. Please notice that the test name has a certain structure <directory_name> are separated with `.` and the function to run (usually with a prefix `test_`) appears after the `:` sign.
+You can find a list of tests in the doc inside the file `ceph_s3_tests_list_single_test.txt`. Please notice that the test name has a certain structure <directory_name> are separated with `/`, the files end with the extention `.py` and the function to run (usually with a prefix `test_`) appears after the `::` sign.
 ## Debug a Single Test (Inside The Tester Pod)
 
 ### 1) Prerequisites:
@@ -174,22 +187,22 @@ Since the file `./src/test/system_tests/ceph_s3_tests/s3-tests/s3tests_boto3/fun
 
 #### B. Temporary change - this change will be saved in the file inside the container, useful when you need a small change.
 You can edit the test by going to the test file and editing the test function. e.g. if you are working on test `s3tests_boto3.functional.test_s3:test_set_bucket_tagging` then you should `vi ./src/test/system_tests/ceph_s3_tests/s3-tests/s3tests_boto3/functional/test_s3.py` and search for the function `test_set_bucket_tagging`.
-## Compare to AWS Response (Inside Tester Pod)
+### Compare to AWS Response (Inside Tester Pod)
 Prerequisites:
 Following the 'Run a Single Ceph S3 Test' steps until 'Deploy The Tester Deployment (Noobaa-Core Tab)'.
 
 In this section we will do some manual changes that will allow you to check AWS response for a specific test (tests that do not use neither ACL nor tenant group).
 1) copy configuration file:
   ```bash
-  cp .src/test/system_tests/ceph_s3_tests/test_ceph_s3_config.conf .src/test/system_tests/ceph_s3_tests/test_ceph_s3_config.conf.aws
+  cp src/test/system_tests/ceph_s3_tests/test_ceph_s3_config.conf src/test/system_tests/ceph_s3_tests/test_ceph_s3_config_aws.conf
   ```
-2) Change the new configuration file to match AWS details, `vi .src/test/system_tests/ceph_s3_tests/test_ceph_s3_config.conf.aws`:
+2) Change the new configuration file to match AWS details, `vi src/test/system_tests/ceph_s3_tests/test_ceph_s3_config_aws.conf`:
 * host = s3.amazonaws.com
 * bucket prefix = choose_name (for example: `bucket prefix = foo-bucket` you will need to manually delete it from AWS, and its name will be `foo-bucket1`, it adds suffix of 1).
 * access_key, secret_key appears 3 times each in the file.
 3) Running tests with the new configuration files will run against AWS:
 ```bash
-S3TEST_CONF=src/test/system_tests/ceph_s3_tests/test_ceph_s3_config.conf.aws tox -c src/test/system_tests/ceph_s3_tests/s3_tests/tox.ini $PWD/src/test/system_tests/ceph_s3_tests/s3-tests/<test_name>
+S3TEST_CONF=${PWD}/src/test/system_tests/ceph_s3_tests/test_ceph_s3_config_aws.conf tox -c src/test/system_tests/ceph_s3_tests/s3-tests/tox.ini ${PWD}/src/test/system_tests/ceph_s3_tests/s3-tests/<test_name>
 ```
 ## Examples
 
@@ -216,60 +229,12 @@ Following the 'Run a Single Ceph S3 Test' steps.
 
 ### 1) Test Pass
 For example: `s3tests_boto3/functional/test_s3.py::test_basic_key_count`
-```
-bash-4.4$ S3TEST_CONF=$PWD/src/test/system_tests/ceph_s3_tests/test_ceph_s3_config.conf tox -c src/test/system_tests/ceph_s3_tests/s3-tests/tox.ini -- --disable-pytest-warnings $PWD/src/test/system_tests/ceph_s3_tests/s3-tests/s3tests_boto3/functional/test_s3.py::test_account_usage
-
-...
-
-====================================================================================== 1 passed, 1 warning in 15.71s =======================================================================================
-_________________________________________________________________________________________________ summary __________________________________________________________________________________________________
-  py: commands succeeded
-  congratulations :)
+![alt text](images/tox_test_success.png)
 
+note that there is the warning:
+`WARNING: could not copy distfile to //.tox/distshare`
+this warning is for tox to use the same dependancies between projects. this feature is depricated and not used on this project. in order to remove the warning you can modify src/test/system_tests/ceph_s3_tests/s3-tests/tox.ini to include the following line in the `[tox]` section: `distshare = /root/node_modules/noobaa-core/.tox/distshare`
 
-```
 ### 2) Test Fail
 For example: `s3tests_boto3/functional/test_s3::test_account_usage.py`
-
-```
-bash-4.4$ S3TEST_CONF=$PWD/src/test/system_tests/ceph_s3_tests/test_ceph_s3_config.conf tox -c src/test/system_tests/ceph_s3_tests/s3-tests/tox.ini src/test/system_tests/ceph_s3_tests/s3-tests/s3tests_boto3/functional/test_s3.
-
-...
-
-================================================================================================= FAILURES =================================================================================================
-____________________________________________________________________________________________ test_account_usage ____________________________________________________________________________________________
-
-...
-
------------------------------------------------------------------------------------------- Captured stdout setup -------------------------------------------------------------------------------------------
-
-...
-
------------------------------------------------------------------------------------------ Captured stdout teardown -----------------------------------------------------------------------------------------
-
-...
-
-========================================================================================= short test summary info ==========================================================================================
-FAILED s3tests_boto3/functional/test_s3.py::test_account_usage - KeyError: 'Summary'
-====================================================================================== 1 failed, 2 warnings in 4.01s =======================================================================================
-ERROR: InvocationError for command '/root/node_modules/noobaa-core/src/test/system_tests/ceph_s3_tests/s3-tests/.tox/py/bin/pytest --disable-pytest-warnings /root/node_modules/noobaa-core/src/test/system_tests/ceph_s3_tests/s3-tests/s3tests_boto3/functional/test_s3.py::test_account_usage' (exited with code 1)
-_________________________________________________________________________________________________ summary __________________________________________________________________________________________________
-ERROR:   py: commands failed
-
-
-FAILED (errors=1)
-
-```
-### 3) Wrong Test Name
-If you will use a test name that not written in the defined structure (as mentioned in 'Test Name' section) you will get a falsy OK.
-
-For example: `s3tests_boto3.functional.test_s3.test_account_usage` instead of `s3tests_boto3.functional.test_s3:test_account_usage` (notice the use of the sign `:` before test).
-```
-bash-4.4$ S3TEST_CONF=$PWD/src/test/system_tests/ceph_s3_tests/test_ceph_s3_config.conf .tox -c src/test/system_tests/ceph_s3_tests/s3-tests/tox.ini src/test/system_tests/ceph_s3_tests/s3-tests/s3tests_boto3/functional/test_s3.py::test_account_usage
-
-----------------------------------------------------------------------
-Ran 0 tests in 0.389s
-
-OK
-```
-You can avoid it by using the name according to the structure or copy the test name from the file `ceph_s3_tests_list_single_test.txt`.
+![alt text](images/tox_test_failed.png)

docs/dev_guide/ceph_s3_tests/images/tox_test_failed.png

@@ -28,6 +28,7 @@ WORKDIR /root/node_modules/noobaa-core/
 #
 ##############################################################
 RUN ./src/test/system_tests/ceph_s3_tests/test_ceph_s3_deploy.sh $(pwd) 
+# add group permissions to s3-tests directory (tox needs it in order to run)
 RUN cd ./src/test/system_tests/ceph_s3_tests/ && \
     chgrp -R 0 s3-tests && \
     chmod -R g=u s3-tests

docs/dev_guide/ceph_s3_tests/images/tox_test_success.png

@@ -74,3 +74,5 @@ s3tests_boto3/functional/test_s3.py::test_object_write_with_chunked_transfer_enc
 s3tests_boto3/functional/test_s3.py::test_versioning_concurrent_multi_object_delete
 s3tests_boto3/functional/test_s3.py::test_post_object_upload_size_rgw_chunk_size_bug
 s3tests_boto3/functional/test_s3.py::test_get_object_torrent
+s3tests_boto3/functional/test_s3select.py::test_count_json_operation
+s3tests_boto3/functional/test_s3select.py::test_column_sum_min_max

src/deploy/NVA_build/Tests.Dockerfile

@@ -15,7 +15,7 @@ const dbg = require('../../../util/debug_module')(__filename);
 dbg.set_process_name('test_ceph_s3');
 const argv = require('minimist')(process.argv.slice(2));
 delete argv._;
-const { S3_CEPH_TEST_SIGV4, CEPH_TEST, DEFAULT_NUMBER_OF_WORKERS } = require('./test_ceph_s3_constants.js');
+const { S3_CEPH_TEST_SIGV4, CEPH_TEST, DEFAULT_NUMBER_OF_WORKERS, TOX_ARGS } = require('./test_ceph_s3_constants.js');
 
 const testing_status = {
     pass: [],
@@ -65,7 +65,7 @@ async function run_s3_tests() {
 
     console.info(`CEPH TEST SUMMARY: Suite contains ${testing_status.total}, ran ${testing_status.pass.length + testing_status.fail.length + testing_status.skip.length} tests, Passed: ${testing_status.pass.length}, Skipped: ${testing_status.skip.length}, Failed: ${testing_status.fail.length}`);
     if (testing_status.skip.length) {
-        console.warn(`CEPH TEST SUMMARY:  ${testing_status.skip.length} skipped tests ${testing_status.skip.join('\n')}`);
+        console.warn(`CEPH TEST SUMMARY:  ${testing_status.skip.length} skipped tests \n${testing_status.skip.join('\n')}`);
     }
     if (testing_status.fail.length) {
         console.error(`CEPH TEST FAILED TESTS SUMMARY: ${testing_status.fail.length} failed tests \n${testing_status.fail.join('\n')}`);
@@ -75,9 +75,8 @@ async function run_s3_tests() {
 
 async function run_all_tests() {
     console.info('Running Ceph S3 Tests...');
-    const tox_args = `-c ${CEPH_TEST.test_dir}${CEPH_TEST.s3_test_dir}${CEPH_TEST.tox_config}`;
     const tests_list_command =
-        `S3TEST_CONF=${process.cwd()}/${CEPH_TEST.test_dir}${CEPH_TEST.ceph_config} tox ${tox_args} -- -q --collect-only --disable-pytest-warnings  2>&1 | awk '{print $1}' | grep test`;
+        `S3TEST_CONF=${process.cwd()}/${CEPH_TEST.test_dir}${CEPH_TEST.ceph_config} tox ${TOX_ARGS} -- -q --collect-only --disable-pytest-warnings  2>&1 | awk '{print $1}' | grep test`;
     try {
         tests_list = await os_utils.exec(tests_list_command, { ignore_rc: false, return_stdout: true });
     } catch (err) {
@@ -100,29 +99,35 @@ async function test_worker() {
     }
 }
 
-async function run_single_test(test) {
+async function run_single_test(test_name) {
     let ceph_args = `S3TEST_CONF=${process.cwd()}/${CEPH_TEST.test_dir}${CEPH_TEST.ceph_config}`;
-    const tox_args = `-c ${CEPH_TEST.test_dir}${CEPH_TEST.s3_test_dir}${CEPH_TEST.tox_config}`;
-    if (S3_CEPH_TEST_SIGV4.includes(test)) {
+    if (S3_CEPH_TEST_SIGV4.includes(test_name)) {
         ceph_args += ` S3_USE_SIGV4=true`;
     }
-    let base_cmd = `${ceph_args} tox ${tox_args}`;
-    if (!S3_CEPH_TEST_OUT_OF_SCOPE_REGEXP.test(test)) {
+    let base_cmd = `${ceph_args} tox ${TOX_ARGS}`;
+    if (!S3_CEPH_TEST_OUT_OF_SCOPE_REGEXP.test(test_name)) {
         try {
-            if (test.includes('boto')) {
-                base_cmd = `${ceph_args} tox ${tox_args} -- -m 'not fails_on_aws'`;
+            if (test_name.includes('boto')) {
+                base_cmd = `${ceph_args} tox ${TOX_ARGS} -- -m 'not fails_on_aws'`;
             }
-            const res = await os_utils.exec(`${base_cmd} ${process.cwd()}/${CEPH_TEST.test_dir}${CEPH_TEST.s3_test_dir}${test}`, { ignore_rc: false, return_stdout: true });
-            if (res.indexOf('SKIP') >= 0) {
-                console.warn('Test skipped:', test);
-                testing_status.skip.push(test);
+            const res = await os_utils.exec(`${base_cmd} ${process.cwd()}/${CEPH_TEST.test_dir}${CEPH_TEST.s3_test_dir}${test_name}`, { ignore_rc: false, return_stdout: true });
+            if (res.indexOf('skipped') >= 0) {
+                console.warn('Test skipped:', test_name);
+                testing_status.skip.push(test_name);
             } else {
-                console.info('Test Passed:', test);
-                testing_status.pass.push(test);
+                console.info('Test Passed:', test_name);
+                testing_status.pass.push(test_name);
             }
         } catch (err) {
-            console.error('Test Failed:', test);
-            testing_status.fail.push(test);
+            // tox will exit with code 1 on error regardless of pytest exit code. pytest exit code 5 means no tests ran.
+            // can happen when 'not fails_on_aws' flag is on for some tests (there are no boto3 tests for the test)
+            if (err.stdout.indexOf("exited with code 5") >= 0) {
+                console.warn('Test skipped:', test_name);
+                testing_status.skip.push(test_name);
+            } else {
+                console.error('Test Failed:', test_name);
+                testing_status.fail.push(test_name);
+            }
         }
     }
 }

src/test/system_tests/ceph_s3_tests/s3-tests-lists/s3_tests_pending_list.txt

@@ -80,7 +80,10 @@ const CEPH_TEST = {
 
 const DEFAULT_NUMBER_OF_WORKERS = 5; //5 was the number of workers in the previous CI/CD process
 
+const TOX_ARGS = `-c ${CEPH_TEST.test_dir}${CEPH_TEST.s3_test_dir}${CEPH_TEST.tox_config}`;
+
 
 exports.S3_CEPH_TEST_SIGV4 = S3_CEPH_TEST_SIGV4;
 exports.CEPH_TEST = CEPH_TEST;
 exports.DEFAULT_NUMBER_OF_WORKERS = DEFAULT_NUMBER_OF_WORKERS;
+exports.TOX_ARGS = TOX_ARGS;