Fix unit tests to run and not error

[stevesloka]

Proposed changes

Fixes tests that were failing and missing data in the various tests.

[github-actions[bot]]

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

[squaremo]

Thank you for fixing up the session unit tests :100:

The controller tests (as opposed to those fixed here) should really be viable to run locally, and I would much rather see as many of the tests run as part of make test as possible rather than splitting them amongst integration tests and unit tests, thereby encouraging people run only a very narrow set of tests locally.

There are some obstacles to total success in that aim, notably

• you need AWS credentials for some older tests (that don't really need to be testing AWS; I just stopped short of rewriting them, because they test several things at once and need some detangling)
• the tests take a while because running pulumi stacks just inherently takes a while -- no getting around that one, really.

Nonetheless, I suggest that this PR can be simplified to fixing up the session struct tests, and making sure they run as part of make test.

[squaremo]

I suggest that this PR can be simplified to fixing up the session struct tests, and making sure they run as part of make test.

@stevesloka With the understanding that you may not have capacity to take this PR further right now -- what do you think about this suggestion?

[github-actions[bot]]

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

[stevesloka]

@squaremo sorry for loosing track of this. I updated to remove the +integration build tags and have the unit tests run with the test target. Please take a look and let me know if you want to make any further changes.

I understand the integration tests need more infra to run, but it would be nice to be able to run unit tests on their own.

[squaremo]

it would be nice to be able to run unit tests on their own

Well kind of. At present the unit tests in ./pkg/... cover very little of the code, while the tests in ./test/ exercise way more. The latter aren't really integration tests, in the sense that they don't in general need a bunch of environment or external services -- they can run locally (modulo either setting up an S3 bucket and AWS creds, or ignoring those failures).

There's little reason to run only the unit tests, since it doesn't tell you much. You can just do go test ./pkg/... -run TestBlah if you want to check a specific test, e.g., while changing some feature.

[github-actions[bot]]

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

[squaremo]

Well kind of. At present the unit tests in ./pkg/... cover very little of the code, while the tests in ./test/ exercise way more. The latter aren't really integration tests, in the sense that they don't in general need a bunch of environment or external services -- they can run locally (modulo either setting up an S3 bucket and AWS creds, or ignoring those failures).

There's little reason to run only the unit tests, since it doesn't tell you much. You can just do go test ./pkg/... -run TestBlah if you want to check a specific test, e.g., while changing some feature.

Having said that, I don't mind the formulation you have in the Makefile, with a separate target, if it prefer that -- please make sure it's not running the same tests again, though, and that's it's marked as phony. I don't think you need test_all if test runs both anyway.

[stevesloka]

I can rip out the unit-test specific bits if you'd like. I do see very little is tested, but figured that would be something new changes would have to add and you'd be setup to have the infra there to run those tests.

Sounds like a better change you'd like to see is to get the ./tests working locally easier?

[squaremo]

a better change you'd like to see is to get the ./tests working locally easier?

That would be really good yeah -- it's nearly there, just

• most of the tests in stack_controller_test.go should be split up so they test one thing at a time;
• most of the tests in stack_controller_test.go could be rewritten to not need AWS, with the exception of testing S3 bucket as a backend (which could be skipped locally)

Fixing the unit tests is a worthwhile change on its own, though, so this PR should stand.

[github-actions[bot]]

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

[stevesloka]

Ok I updated to have this just fixup the tests. But they aren't ever run so not sure what value this does for us. =)

I can look to get the other bits running locally, or if you have other plans for that I can leave it to you. We could review the change that needs to happen in a new issue and verify the work steps.

[squaremo]

Sorry, I was unclear -- I think it's fine to include running the unit tests in the Makefile, either as another command in the recipe for test, or in another phony target that test depends on. You already fixed the bit I was concerned about, which was that the unit_test target ran the tests under ./tests/ again.