Skip to content

Add more unit tests + enable unit testing in CI#3

Merged
johnflavin merged 12 commits into
developfrom
unit-tests
Jun 10, 2026
Merged

Add more unit tests + enable unit testing in CI#3
johnflavin merged 12 commits into
developfrom
unit-tests

Conversation

@johnflavin

@johnflavin johnflavin commented Jun 9, 2026 •
edited
Loading

Copy link
Copy Markdown
  • Add unit tests for filters, user details, and token handling
  • Add GH Actions workflow to run unit tests on pushes/merges + PRs to main and develop
  • Remove old unused CircleCI config

johnflavin and others added 9 commits June 9, 2026 10:01
@johnflavin @claude
Add unit tests for PKCE provider, user details, and login extension
55dddd1 
Adds 18 isolated unit tests (Mockito only, no live OIDC/DB/Spring context):

- PkceAuthorizationCodeAccessTokenProviderTest (7): authorization-redirect
  parameter building, S256 code_challenge = BASE64URL(SHA-256(verifier)),
  PKCE-disabled path, CSRF rejection on missing state, code_verifier replay
  at the token endpoint, and configurable state-key length.
- OpenIdConnectUserDetailsTest (6): username-pattern resolution (default,
  custom multi-claim, missing-claim error) and email/name claim mapping.
- OpenIdLoginExtensionTest (5): one-shot error-message display/clear and the
  blank/absent/no-data/exception short-circuits.

Raises coverage from 46%/26% to 57%/37% (instruction/branch); the three
targeted classes reach 95%/94%/100% instruction coverage.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@johnflavin @claude
Add unit tests for OpenIdConnectFilter decision logic
45e415b 
Covers the filter's pure logic that previously had no direct tests, all
without an OAuth2 exchange, database, or servlet container (filter built with
mocked collaborators; private methods exercised via reflection):

- Email-domain whitelisting (isAllowedEmailDomain / shouldFilterEmailDomains):
  filtering disabled, on/off whitelist, case-insensitive match, "*" wildcard,
  malformed address with no domain, unknown provider, and default-off config.
- Login-page error mapping (getUserMessage): usernamePattern config error vs.
  generic retry message for other failures.
- ID-token encryption detection (isIdTokenEncrypted): 5-part JWE vs 3-part JWS.

Raises OpenIdConnectFilter coverage from 38%/24% to 56%/54%
(instruction/branch) and overall coverage to 63%/46%. The static-XNAT paths
(Users.createUser, TurbineUtils redirects) remain for integration testing.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@johnflavin @claude
Add GitHub Actions CI workflow to run tests on JDK 21
8a9c7fc 
Runs `./gradlew test jacocoTestReport` on a Temurin JDK 21 runner for pushes
and PRs to main, with Gradle dependency caching. Uploads JUnit results and the
JaCoCo HTML report as build artifacts.

JDK 21 matches the build's sourceCompatibility/targetCompatibility (the Java 21
/ XNAT 1.10.0 migration). The legacy .circleci/config.yml is pinned to JDK 8
and cannot compile the current codebase.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@johnflavin @claude
Remove legacy CircleCI configuration
a0c8b9f 
The CircleCI pipeline was pinned to JDK 8 (circleci/openjdk:8-jdk-buster),
which cannot compile the Java 21 codebase, and the circleci/* image namespace
is deprecated. It is superseded by the GitHub Actions workflow added in the
previous commit. Also removes the CircleCI-only Maven settings.xml (env-var
placeholders, no secrets) that was used solely by the removed publish step.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@johnflavin @claude
Remove generated Maven settings before running tests in CI
861ac34 
actions/setup-java writes a default ~/.m2/settings.xml whose server
credentials the net.linguica.maven-settings plugin fails to decrypt at
configuration time, breaking the test-only CI job. Remove the file
before invoking Gradle.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@johnflavin @claude
Bump GitHub Actions to latest major versions
ef18006 
Updates checkout v4->v6, setup-java v4->v5, and upload-artifact v4->v7
to clear the Node.js 20 deprecation warning and run on Node.js 24.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@johnflavin @claude
Run CI on pull requests to develop
ede98e4 
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@johnflavin @claude
Run CI only on PRs to main and develop
8be3183 
Drop the push trigger on all branches so opening a PR no longer runs CI
twice against the same commit.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@johnflavin @claude
Also run CI on pushes/merges to main and develop
9088e5c 
Scoped to main/develop so merges get a post-merge run without
double-running on PR branches.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kathrynalpert
kathrynalpert approved these changes Jun 9, 2026
View reviewed changes

@kathrynalpert kathrynalpert left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It'd be nice to get more detail about the test results in the Actions log without having to download the artifacts - you can do this by having gradle print more info (some examples of this in Scout), or presumably there's a marketplace action that'd render a report from the XML...

@kathrynalpert

kathrynalpert commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

This is probably out of scope, but while you're working on CI/removing circleCI, should there be a job to publish to Jfrog on merge?

@johnflavin

johnflavin commented Jun 10, 2026

Copy link
Copy Markdown
Author

This is probably out of scope, but while you're working on CI/removing circleCI, should there be a job to publish to Jfrog on merge?

I did consider this, but I do think that's a separate thing. It might be nice if there were a standard way to do this for all the plugin repos; I'll start a conversation about that.

johnflavin and others added 3 commits June 10, 2026 08:53
@johnflavin @claude
Render test results in the CI run summary
5f16324 
Add test-summary/action to read Gradle's JUnit XML and display a results
table on the workflow run summary page. Matches the setup in the scout
repo (same action, same pinned v2.6 SHA).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@johnflavin @claude
Log per-test results to the console during the test task
035fb8e 
Gradle's test task is silent by default. Add a testLogging block so CI
shows live pass/skip/fail progress and full stack traces on failure,
and forwards test stdout/stderr to the build output.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@johnflavin @claude
Float test-summary action to v2 instead of a pinned SHA
bb9ebeb 
This repo has no automation to bump pinned action SHAs, so track the v2
major tag to pick up fixes automatically. Drop the Scout reference from
the rationale comment.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@johnflavin johnflavin merged commit 26026b0 into develop Jun 10, 2026
1 check passed
@johnflavin johnflavin deleted the unit-tests branch June 10, 2026 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kathrynalpert kathrynalpert kathrynalpert approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@johnflavin @kathrynalpert