Automated Testing#62
Conversation
This reverts commit ba7a25f.
There was a problem hiding this comment.
why do tests get their own go.mod?
There was a problem hiding this comment.
Basically trying to isolate them so that a) the dependencies that are there just for integration tests don't leak into the main module; b) it makes the CI a little more straightforward when we want to run integration against two different versions of temporal server.
That said, since I went back on the original idea of actually mocking out portions of the AWS API, I don't think I'm going to end up introducing anything huge into the dependency graph like a Testcontainers or anything. Let me play around with what it would look like to simplify it
There was a problem hiding this comment.
It's generally good practice for testing-only packages to have their own go.mod, so I support this approach. That said, I'd make the package internal/testing instead of tests at the top-level.
There was a problem hiding this comment.
I think for now I'm going to leave them where they're at - internal makes more sense if we didn't do the multi-module approach, since that would prevent what's in there from being imported elsewhere; as it is though, since the tests are in their own module, they're already not importable and we'd just be nesting for not a ton of benefit IMO
There was a problem hiding this comment.
I don't quite see how this is a 'testing-only package' as it is more the testing package for the module it is in. @jaypipes looking to learn more: what is the motivation for the practice/what is the problem it is trying to solve?
| err = env.Client.DeleteWorkerControllerInstance(ctx, nsEntry, version, "test-identity") | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
should there be another exists check after this?
There was a problem hiding this comment.
Works for me yeah
jaypipes
left a comment
jaypipes
left a comment
There was a problem hiding this comment.
👍 The integration tests are a lot more valuable than the unit tests (IMHO) and generally easier to reason about and extend. So, +1 from me.
There was a problem hiding this comment.
It's generally good practice for testing-only packages to have their own go.mod, so I support this approach. That said, I'd make the package internal/testing instead of tests at the top-level.
| - name: Checkout | ||
| uses: actions/checkout@v7 | ||
| with: | ||
| submodules: recursive |
There was a problem hiding this comment.
The submodules here refers to Git submodules, not Go modules. Unless you have a very good reason, I'd avoid Git submodules like the plague...
There was a problem hiding this comment.
Ah yeah good call. No reason for git submodules in here at all, I'll nuke that
|
|
||
| // emitProviderEvent reports an action to the observer registered for the | ||
| // request's deployment build, if any. | ||
| func emitProviderEvent(rc RequestContext, action string) { |
There was a problem hiding this comment.
should this have a more specific name to avoid it colliding with non-test code?
There was a problem hiding this comment.
Yeah I think we can make this emitTestProviderEvent which should avoid any potential collision and be a little more specific about intent
There was a problem hiding this comment.
why is this in it's own file?
There was a problem hiding this comment.
So that we can put the test_dep build tag on and prevent non-test code from attaching an observer here. My preference would probably have been to just have a separate ComputeProvider implementation altogether but it has to pass the validity check (be part of the enum of available Providers) so we can't just put an arbitrary one in there.
3 participants
What was changed
Adds automated testing on main and PR branches.
Linting
Adds a golangci-lint configuration synthesized from other core repos in the organization. Linters run only on code changed within the instant pull request. Linters do not run on the main branch.
Unit Testing
Unit tests run on all PR and main builds.
We collect but do not currently do anything with code coverage data. In the future we may want to integrate with Codecov, in line with some of our other projects.
Integration Testing
Integration tests use the server test environment from the OSS server repo, with the WCI component injected in. Integration tests are run against both the pinned version of the server (as defined in
go.mod) as well as the currentmainbranch (these are advisory and will not fail the build).Currently the only defined integration test is a simple smoke test to ensure we can create a worker deployment version, this will be expanded in future work.
Integration tests live in a nested submodule for dependency separation etc; there is a CI job configured to enforce that the versions defined in the respective
go.modfiles for the go language version as well as the temporal server versions stay in sync. This check will fail the build, in order to prevent drift.Why?
Enforces code standards as well as ensuring ongoing validation for the codebase.