12367 GitHub actions workflow for jenkins redundancy#12368
12367 GitHub actions workflow for jenkins redundancy#12368srmanda-cs wants to merge 12 commits intoIQSS:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new GitHub Actions workflow intended to run the same API/integration test suite as Jenkins, but against a Dataverse stack started via the Maven container tooling, improving CI redundancy and visibility of failures.
Changes:
- Added
.github/workflows/container_integration_tests.ymlto build/start the container stack, runtests/integration-tests.txt, and publish test artifacts/results. - Updated
SearchITto search for the dataset via theid:dataset_<id>query pattern. - Disabled
testRetrieveMyDataCollectionsinDataRetrieverApiIT.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
src/test/java/edu/harvard/iq/dataverse/api/SearchIT.java |
Adjusts dataset search query to use id:dataset_<id> format. |
src/test/java/edu/harvard/iq/dataverse/api/DataRetrieverApiIT.java |
Disables a failing/unstable test method. |
.github/workflows/container_integration_tests.yml |
Introduces a new container-based integration test workflow, including reporting and artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # The pom.xml automatically merged the files and put them here: | ||
| mvn coveralls:report \ | ||
| -P all-unit-tests \ | ||
| -DrepoToken=$COVERALLS_REPO_TOKEN \ | ||
| -DjacocoReports=target/site/jacoco-merged-test-coverage-report/jacoco.xml |
There was a problem hiding this comment.
This is probably wrong, but let me verify just in case.
There was a problem hiding this comment.
This is right, JaCoCo reporting is failing
There was a problem hiding this comment.
Is this related to why I don't see this branch over at https://coveralls.io/github/IQSS/dataverse ? Check this out:
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Updated concurrency group name to include branch reference and removed .txt files from paths-ignore.
Removed .txt files from paths-ignore in workflow.
Temporarily disable unreliable integration test for retrieving data collections.
Removed force_run input from workflow dispatch.
Replace curl command with docker cp for SUSHI config file.
Replaced local file copy with curl command to fetch SUSHI config.
pdurbin
left a comment
pdurbin
left a comment
There was a problem hiding this comment.
This is fantastic! Thanks, @srmanda-cs! ❤️
I'm leaving a few comments and will co-assign you.
| - ".github/*.md" | ||
| pull_request: | ||
| branches: | ||
| - develop |
There was a problem hiding this comment.
Should we add "master" here? That way the tests will run when we release, I would think, after this step: https://guides.dataverse.org/en/6.10.1/developers/making-releases.html#merge-develop-into-master-non-hotfix-only
Here's an example of where we merged develop into master:
| push: | ||
| branches: | ||
| - develop | ||
| paths-ignore: | ||
| - "doc/**" | ||
| - "**/*.md" | ||
| - ".github/ISSUE_TEMPLATE/**" | ||
| - ".github/*.md" |
There was a problem hiding this comment.
Do we need "push" at all? We have branch protection turned on for both "develop" and "master".
| DVAPIKEY: "burrito" | ||
| DV_APIKEY: "burrito" | ||
| DV_API_KEY: "burrito" |
There was a problem hiding this comment.
Do we need all three of these? 🌯 🌯 🌯
| run: | | ||
| set -euo pipefail | ||
| # Fixes MakeDataCountApiIT | ||
| docker exec dev_dataverse sh -c 'curl https://raw.githubusercontent.com/IQSS/dataverse/develop/src/test/java/edu/harvard/iq/dataverse/makedatacount/sushi_sample_logs.json > /tmp/sushi_sample_logs.json && head /tmp/sushi_sample_logs.json' |
There was a problem hiding this comment.
I guess this will work but would it make more sense to copy the sushi_sample_logs.json file locally rather than downloading it from GitHub?
| # The pom.xml automatically merged the files and put them here: | ||
| mvn coveralls:report \ | ||
| -P all-unit-tests \ | ||
| -DrepoToken=$COVERALLS_REPO_TOKEN \ | ||
| -DjacocoReports=target/site/jacoco-merged-test-coverage-report/jacoco.xml |
There was a problem hiding this comment.
Is this related to why I don't see this branch over at https://coveralls.io/github/IQSS/dataverse ? Check this out:
| } | ||
|
|
||
| // Test getting a list of collections that the user can add datasets to | ||
| @Disabled("Temporarily disabled because this integration test is not reliable in CI; re-enable once stabilized. All assertions return one extra dataset than expected.") |
There was a problem hiding this comment.
Once we merge this PR, we plan to re-enable this test. See the following:
What this PR does / why we need it:
Add a GitHub Actions Workflow that replicates the exact same tests that Jenkins run with the same configurations, but with containers instead
Which issue(s) this PR closes:
Special notes for your reviewer:
All your suggestions and criticism are deeply welcome, and since it is a major workflow I'm willing to work on it again and again until it meets Dataverse's needs accurately and addresses any concerns the team has. There will probably be a lot, since this is a prototype workflow that I quickly hacked together in a few weeks.
Suggestions on how to test this:
Clicking run workflow should do it.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
N/A
Is there a release notes update needed for this change?:
N/A
Additional documentation:
Depends on where and how you would want me to document it.