Skip to content

chore: add all other languages, latest major versions, many more tests to test-server#134

Merged
kessplas merged 224 commits into
stagingfrom
kess/fix-ci
Feb 4, 2026
Merged

chore: add all other languages, latest major versions, many more tests to test-server#134
kessplas merged 224 commits into
stagingfrom
kess/fix-ci

Conversation

@kessplas

@kessplas kessplas commented Jan 21, 2026

Copy link
Copy Markdown
Contributor

Issue #, if available: #125, #127

Description of changes: This PR merges in the many changes made to the test-server directory during the course of the project to add key commitment to all of the S3EC implementations. In addition to the pre-existing changes for this project, the following additional changes have been made:

  • pin Ruby 3.4.7 since the newer 3.4.8 causes biginteger to fail native compilation
  • move all submodules off of private staging repos and on to the latest commit in the public repos
    • remove the C++ PAT, since it has expired
    • do NOT remove the Ruby PAT, since we still need it for the private spec repo, for now
  • remove all "current" test servers, since "transition" is now current given key commitment is released
  • remove the examples, as they are not needed for Python development
  • dynamically load Java package version from the submodule's pom
  • some other minor / cosmetic changes such as renaming the TestServer tests

Notes for reviewers:

  • The primary purpose of review here should be considering whether or not the test-server updates are being added to the staging (in-progress S3EC Python) branch in a reasonable way, e.g. without disrupting the existing repo.
  • As such, I recommend focusing primarily on changed files rather than new files.
  • Most of the test-server code itself can be ignored. This code was reviewed independently and has not changed significantly. You may want to inspect the diff between kess/fix-ci and fireegg-test-servers to confirm this.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@kessplas kessplas changed the base branch from fireegg-test-servers to staging January 30, 2026 22:27
@kessplas kessplas changed the title Kess/fix ci chore: add all other languages, latest major versions, many more tests to test-server Jan 30, 2026
@kessplas kessplas marked this pull request as ready for review January 31, 2026 00:34
@kessplas kessplas requested a review from texastony January 31, 2026 00:36
@texastony

Copy link
Copy Markdown

Has the decision to include the test server here already been legislated?

If so, know that I disagree, but am willing to commit.

If not, IMO the test-server should be in a distinct repo.
A repo that is invoked by
all S3EC repos as part of Continuous Deployments (merges to main) and
maybe other CI targets,
but maybe not as part of daily development.

IMO We can make contributing to our open source projects easy to do, hard to do poorly.
Baking the test server in with the Python implementation complicates:

  • Pulling the repo down (lots of submodules)
  • The Git history of the repo (was a change for S3EC Python or the test server or some part of the test server)

While every unique repo has a cost to it's existence
(some amount of CI, metadata maintenance [who can write to it], Issues, etc.)
I believe adding the test server here has too high cost in terms of easy to do and makes it medium to do poorly.

We can mitigate the costs of unique repo by disabling public issue creation,
using gh and terra-form to automate repo metadata configurations,
and other automation steps.

@texastony texastony left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I have requested some changes.
They are all about naming.
I have reviewed only 5 files, and some of the Makefile.
The 5 I reviewed had changes, so I targeted those instead of new files or removed files.

Comment thread .github/workflows/all-ci.yml
Comment thread .github/workflows/duvet.yml Outdated
Comment thread .github/workflows/duvet-test-server.yml
Comment thread .github/workflows/lint.yml
Comment thread cdk/lib/cdk-stack.ts
Comment thread test-server/model/client.smithy
Comment thread test-server/Makefile Outdated

# CI target for GitHub Actions
ci: start-servers run-tests stop-servers
ci:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Strong Suggestion: Align GitHub workflow file names and job names with these Makefile targets.
Make readers life easy; one lexicon to rule them all.

@kessplas kessplas Feb 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In general I agree but I disagree that that logic applies here.
It's a different context;
this Makefile exposes a general CI target for the TestServer.
The S3EC Python CI runs the TestServer's CI.
But the TestServer could exist elsewhere and should not depend on the S3EC Python's convention.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Eh, your other comment makes more sense, so we should do this.

Comment thread test-server/Makefile Outdated

# Default target
all: start-servers run-tests
.PHONY: all start-servers run-tests stop-servers clean ci check-env help

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Strong Suggestion: Align these Makefile targets with GitHub workflows.
And prefix them all with a consistent prefix like test-server.

With Smithy-Dafny's recursive Makefile design,
I would regularly get confused about what Makefile target
did which event
and where it was defined.

This Makefile is nested;
there is another Makefile at the root of the repo,
which may end up citing this one.

Prepare for a complicated future with a simple solution.

Suggested change
.PHONY: all start-servers run-tests stop-servers clean ci check-env help
.PHONY: test-servers-all test-servers-start test-servers-run-tests test-servers-stop test-servers-clean test-servers-ci test-servers-check-env test-servers-help

That, or make the Test-Server a repo of it's own.
NOTE: I strongly prefer making the test-server a repo of it's own, but will disagree and commit.

@kessplas kessplas merged commit 0cfd218 into staging Feb 4, 2026
8 checks passed
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.

8 participants