chore: add all other languages, latest major versions, many more tests to test-server#134
Conversation
…s3/RoundTripTests.java Co-authored-by: Kess Plasmeier <76071473+kessplas@users.noreply.github.com>
…s3/RoundTripTests.java Co-authored-by: Kess Plasmeier <76071473+kessplas@users.noreply.github.com>
…client-python into go-test-server
…s3-encryption-client-python into rishav/dotnet/testserver
|
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. IMO We can make contributing to our open source projects easy to do, hard to do poorly.
While every unique repo has a cost to it's existence We can mitigate the costs of unique repo by disabling public issue creation, |
texastony
left a comment
There was a problem hiding this comment.
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.
|
|
||
| # CI target for GitHub Actions | ||
| ci: start-servers run-tests stop-servers | ||
| ci: |
There was a problem hiding this comment.
Strong Suggestion: Align GitHub workflow file names and job names with these Makefile targets.
Make readers life easy; one lexicon to rule them all.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Eh, your other comment makes more sense, so we should do this.
|
|
||
| # Default target | ||
| all: start-servers run-tests | ||
| .PHONY: all start-servers run-tests stop-servers clean ci check-env help |
There was a problem hiding this comment.
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.
| .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.
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:
bigintegerto fail native compilationNotes for reviewers:
staging(in-progress S3EC Python) branch in a reasonable way, e.g. without disrupting the existing repo.kess/fix-ciandfireegg-test-serversto 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.