Refactor tests. #92
Draft
RodneyCodess wants to merge 88 commits into
Draft
Conversation
aaronweeden
reviewed
Jun 3, 2026
| name: Spin up containers and test different Python versions against different XDMoD web server versions | ||
| command: ./tests/ci/scripts/run-tests.sh | ||
| name: Build XDMoD web server images | ||
| command: bash ./tests/ci/scripts/build_xdmod_img.sh "<< parameters.xdmod_version >>" |
There was a problem hiding this comment.
Instead of providing "<< parameters.xdmod_version >>" as an argument and setting XDMOD_VERSION="$1" in the script, I think it would be simpler to set it as an environment variable using environment; see here for an example.
aaronweeden
reviewed
Jun 3, 2026
| at: . | ||
| - run: | ||
| name: Run tests against XDMoD web server image | ||
| command: bash ./tests/ci/scripts/run_tests.sh "<< parameters.xdmod_version >>" "<< parameters.python_version >>" |
aaronweeden
reviewed
Jun 11, 2026
| @@ -0,0 +1,56 @@ | |||
| #!/bin/bash | |||
| set -eo pipefail | |||
There was a problem hiding this comment.
Suggested change
| set -eo pipefail | |
| set -exo pipefail |
aaronweeden
reviewed
Jun 11, 2026
| @@ -0,0 +1,56 @@ | |||
| #!/bin/bash | |||
| set -eo pipefail | |||
There was a problem hiding this comment.
Suggested change
| set -eo pipefail | |
| set -exo pipefail |
aaronweeden
reviewed
Jun 17, 2026
| at: . | ||
| - run: | ||
| name: Run tests against XDMoD web server image | ||
| command: bash ./tests/ci/scripts/run_tests.sh |
There was a problem hiding this comment.
Suggested change
| command: bash ./tests/ci/scripts/run_tests.sh | |
| command: ./tests/ci/scripts/run_tests.sh |
aaronweeden
reviewed
Jun 17, 2026
| name: Spin up containers and test different Python versions against different XDMoD web server versions | ||
| command: ./tests/ci/scripts/run-tests.sh | ||
| name: Build XDMoD web server images | ||
| command: bash ./tests/ci/scripts/build_xdmod_img.sh "<< parameters.xdmod_version >>" |
There was a problem hiding this comment.
Suggested change
| command: bash ./tests/ci/scripts/build_xdmod_img.sh "<< parameters.xdmod_version >>" | |
| command: ./tests/ci/scripts/build_xdmod_img.sh "<< parameters.xdmod_version >>" |
aaronweeden
reviewed
Jun 17, 2026
| at: . | ||
| - run: | ||
| name: Generate coverage report | ||
| command: bash ./tests/ci/scripts/generate_coverage_report.sh |
There was a problem hiding this comment.
Suggested change
| command: bash ./tests/ci/scripts/generate_coverage_report.sh | |
| command: ./tests/ci/scripts/generate_coverage_report.sh |
aaronweeden
requested changes
Jun 18, 2026
| name: Spin up containers and test different Python versions against different XDMoD web server versions | ||
| command: ./tests/ci/scripts/run-tests.sh | ||
| name: Build XDMoD web server images | ||
| command: ./tests/ci/scripts/build_xdmod_img.sh "<< parameters.xdmod_version >>" |
There was a problem hiding this comment.
Suggested change
| command: ./tests/ci/scripts/build_xdmod_img.sh "<< parameters.xdmod_version >>" | |
| command: ./tests/ci/scripts/build_xdmod_img.sh |
If you set the environment variable XDMOD_VERSION above then you don't need to pass it in here.
Comment on lines
+13
to
+15
| parameters: | ||
| xdmod_version: | ||
| type: string |
There was a problem hiding this comment.
Suggested change
| parameters: | |
| xdmod_version: | |
| type: string | |
| parameters: | |
| xdmod_version: | |
| type: string | |
| environment: | |
| XDMOD_VERSION: << parameters.xdmod_version >> |
Add this environment variable that can be used later.
Comment on lines
+84
to
+85
|
|
||
|
|
There was a problem hiding this comment.
Suggested change
Remove these two blank lines.
| @@ -0,0 +1,56 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Suggested change
| #!/bin/bash | |
| #!/bin/bash |
Comment on lines
+38
to
+45
| rest_token=$(curl --cacert "$(pwd)/localhost.crt" -sS -X POST -c xdmod.cookie -d 'username=normaluser&password=normaluser' https://localhost:8080/rest/auth/login | jq -r '.results.token') | ||
| api_token=$(curl --cacert "$(pwd)/localhost.crt" -sS -X POST -b xdmod.cookie "https://localhost:8080/rest/users/current/api/token?token=$rest_token" | jq -r '.data.token') | ||
|
|
||
| # write the token where the tests read it | ||
| echo "XDMOD_API_TOKEN=$api_token" > ~/.xdmod-data-token | ||
|
|
||
| # trust the cert for requests | ||
| cat "$(pwd)/localhost.crt" >> "$(python3 -c 'import certifi; print(certifi.where())')" |
There was a problem hiding this comment.
Suggested change
| rest_token=$(curl --cacert "$(pwd)/localhost.crt" -sS -X POST -c xdmod.cookie -d 'username=normaluser&password=normaluser' https://localhost:8080/rest/auth/login | jq -r '.results.token') | |
| api_token=$(curl --cacert "$(pwd)/localhost.crt" -sS -X POST -b xdmod.cookie "https://localhost:8080/rest/users/current/api/token?token=$rest_token" | jq -r '.data.token') | |
| # write the token where the tests read it | |
| echo "XDMOD_API_TOKEN=$api_token" > ~/.xdmod-data-token | |
| # trust the cert for requests | |
| cat "$(pwd)/localhost.crt" >> "$(python3 -c 'import certifi; print(certifi.where())')" | |
| rest_token=$(curl --cacert "localhost.crt" -sS -X POST -c xdmod.cookie -d 'username=normaluser&password=normaluser' https://localhost:8080/rest/auth/login | jq -r '.results.token') | |
| api_token=$(curl --cacert "localhost.crt" -sS -X POST -b xdmod.cookie "https://localhost:8080/rest/users/current/api/token?token=$rest_token" | jq -r '.data.token') | |
| # write the token where the tests read it | |
| echo "XDMOD_API_TOKEN=$api_token" > ~/.xdmod-data-token | |
| # trust the cert for requests | |
| cat "localhost.crt" >> "$(python3 -c 'import certifi; print(certifi.where())')" |
The $(pwd) should be unnecessary.
|
|
||
| REQUESTS_CA_BUNDLE=localhost.crt XDMOD_HOST="https://localhost:8080" python3 -m pytest --cov --cov-branch -vvs -o log_cli=true tests/ | ||
|
|
||
| mv .coverage ".coverage.${PYTHON_VERSION}.${XDMOD_VERSION}" No newline at end of file |
| data, | ||
| dtype='string', | ||
| index_col=0, | ||
|
|
There was a problem hiding this comment.
Suggested change
Remove empty line.
Comment on lines
+75
to
+97
| @@ -94,7 +94,7 @@ def __get_data_dir(override_default_data=False): | |||
| ], | |||
| }, | |||
| }, | |||
| '33346', | |||
| ('33346', '33346'), | |||
There was a problem hiding this comment.
Suggested change
| '54749', | |
| 'raw-data-every-1000-no-fields-no-filters.csv', | |
| ), | |
| ( | |
| @@ -94,31 +94,34 @@ | |
| ], | |
| }, | |
| }, | |
| '33346', |
I would set it to just the new number and move the comment later; see below.
Comment on lines
+122
to
+124
| xdmod_main_number, xdmod_11_number = number | ||
| expected_number = xdmod_main_number if XDMOD_VERSION in ['xdmod-11-0', 'xdmod-11-0-dev'] else xdmod_11_number | ||
| assert 'Got ' + expected_number + ' rows...DONE' in capsys.readouterr().out |
There was a problem hiding this comment.
Suggested change
| xdmod_main_number, xdmod_11_number = number | |
| expected_number = xdmod_main_number if XDMOD_VERSION in ['xdmod-11-0', 'xdmod-11-0-dev'] else xdmod_11_number | |
| assert 'Got ' + expected_number + ' rows...DONE' in capsys.readouterr().out | |
| # This PR added an extra job: https://github.com/ubccr/xdmod/pull/2176 | |
| if XDMOD_VERSION in ['xdmod-11-0', 'xdmod-11-0-dev']: | |
| number -= 1 | |
| assert 'Got ' + number + ' rows...DONE' in capsys.readouterr().out |
aaronweeden
reviewed
Jun 18, 2026
Comment on lines
+23
to
+24
| python3 -m pip install --upgrade flake8 flake8-commas flake8-quotes | ||
| python3 -m flake8 . --max-complexity=10 --max-line-length=160 --show-source --exclude __init__.py |
There was a problem hiding this comment.
I would suggest we move linting into a separate workflow in .circleci/config.yml so that if linting fails it doesn't fail the entire workflow — this can be annoying when developing a PR.
We could have the current workflow workflow be renamed to build_and_test and the new one be called lint.
Co-authored-by: Aaron Weeden <31246768+aaronweeden@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Motivation and Context
Tests performed
Type of change:
Checklist:
xdmod-notebooksrepository as necessary, and the notebooks all run successfully