Skip to content

Conversation

@bieryAtFnal
Copy link
Contributor

Description

This change is correlated with DUNE-DAQ/integrationtest#138.

In recent discussions about improvements that people would like to see in the available regression tests, it was mentioned that it would be helpful if the log files, etc. from failed tests did not disappear as quickly as they sometimes do.

We looked into options that are available with pytest, but it didn't seem possible to save directories from failed tests longer than ones from successful tests.

So, we have added logic to our "bundle" scripts to do this. (initially, just the daqsystemtest bundle script)

The complication with doing that, is that it is not obvious how to get a run of the pytest program to return the name of the current pytest subdirectory (e.g. /tmp/pytest-of-${USER}/pytest-1234).

One idea was to have the integrationtest infrastructure code print the pytest output directory name to the console, capture that in the bundle script, etc. Unfortunately, that wouldn't work when we have the --concise-output option enabled in the bundle script (the purpose of that option is to suppress the console output).

The idea that I implemented was to have the bundle script pass some unique keys to the integrationtest infrastructure, have the integrationtest infrastructure write that information to a file in the pytest output directory, and have the bundle script look for a file with the relevant keys to find which pytest subdir corresponds to the most recent test.

Once the bundle script knows the correct pytest output directory, it is easy to make a copy of it.

The bundle script also now has logic to clean up old "failed" pytest output dirs (older than 26 hours or more than 10 failed directories).

Here are suggested steps for testing these changes. These steps locally modified the readout_type_scan_test so that it often/always fails, and with that, we can see the creation of "failed" pytest output directories happening.

DATE_PREFIX=`date '+%d%b'`
TIME_SUFFIX=`date '+%H%M'`

source /cvmfs/dunedaq.opensciencegrid.org/setup_dunedaq.sh
setup_dbt latest
dbt-create -n NFD_DEV_251216_A9 ${DATE_PREFIX}FDDevTest_${TIME_SUFFIX}
cd ${DATE_PREFIX}FDDevTest_${TIME_SUFFIX}/sourcecode

git clone https://github.com/DUNE-DAQ/daqsystemtest.git -b kbiery/copy_failed_integtest_dirs
cd ..

dbt-workarea-env

git clone https://github.com/DUNE-DAQ/integrationtest.git -b kbiery/copy_failed_integtest_dirs
cd integrationtest; pip install -U . ; cd ..

dbt-build -j 12
dbt-workarea-env

sed -i 's,"min_size_bytes": 7272,"min_size_bytes": 7273,' sourcecode/daqsystemtest/integtest/readout_type_scan_test.py

daqsystemtest_integtest_bundle.sh -k readout_type_scan --stop-on-fail --concise-output
echo ""
echo -e "\U1F535 \U2705 Note that when the regression test failed, a copy of the pytest output directory was created. \U2705 \U1F535"

Type of change

  • New feature or enhancement (non-breaking change which adds functionality)

Testing checklist

  • Minimal system quicktest passes (pytest -s minimal_system_quick_test.py)
  • Full set of integration tests pass (daqsystemtest_integtest_bundle.sh)

Further checks

  • Code is commented where needed, particularly in hard-to-understand areas

… the logfile directories for failed tests.
Copy link
Contributor

@wesketchum wesketchum left a comment

Choose a reason for hiding this comment

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

LGTM. I followed the instructions and saw the failed test directories get created. I additionally ran a test with some passing/some failing, and saw the behavior I would expect (passing tests logs as they have been, failing test logs get copied).

The one minor thing is that I see some of the symbolic links for configcurrent and runcurrent are pointing to the original pytest dir (not the copied one), so at some point they may become invalid, but I think that's ok as those aren't relevant when doing historical review of tests. We could consider not copying or removing those soft links just to avoid any potential confusion.

@bieryAtFnal
Copy link
Contributor Author

Good idea. I've added a couple of lines that remove the symbolic links in the copied directory.

@MRiganSUSX
Copy link
Contributor

Hi @bieryAtFnal,
I can also confirm that this works as expected. I do have some suggestions for improvement - nothing major, but nice to have.

-> Bash script:

  • it's a good practice to enclose vars in quotes (" "), I agree that this works flawlessly in the current setup, but this adds some additional defense mechanism in cases where the setup may not be perfect. Examples:
cp -pR ${pytest_tmpdir} ${new_dir}  -> cp -pR "${pytest_tmpdir}" "${new_dir}"
rm -rf ${test_dir} -> rm -rf "${test_dir}"
for test_dir in ${test_dirs_to_remove[@]}; do -> for test_dir in "${test_dirs_to_remove[@]}"; do

and more.

  • we could improve the find command as it could break on spaces, newlines, weird characters. An example using mapfile without the use of ls:
mapfile -t bundle_info_files < <(
  find "/tmp/pytest-of-${USER}" -type f -name "bundle_script_info.json" \
       -printf '%T@ %p\n' \
  | sort -nr \
  | awk '{print $2}'
)

-> Python:

  • there could be additional error handling for cases like malformed env var, json write fail
  • json formatting: using json.dump will add easier to parse output:
info_file.write(f"{json.dumps(bundle_info_data)}") -> json.dump(bundle_info_data, info_file, indent=2)

As mentioned, none of this is needed but suggestions for improvements if you like them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants