Differential D4752
[services] Add reset of the local cloud before unit tests Authored by varun on Aug 5 2022, 4:54 AM. Tags None Referenced Files
Details To properly run unit tests from the local sandbox and using localcloud we should call reset the localcloud before the unit tests are running in case of the database schema was changed and to clean the test data. Related Linear task: ENG-1491 Running of yarn run-unit-tests *service-name* command also resets the localstack.
Diff Detail
Event TimelineComment Actions Sorry, but I don't really understand why we have to do that.
Why would that happen? I think these tests should work on both, the clean cloud and the modified one. This comment was removed by • karol. Comment Actions When the database schema was changed in Terraform script, the tests will fail. We need to update it manually by running init-local-cloud on the CI side, as long as on the local dev environment too. Running this automatically before sandbox unit tests will not require running it manually on the local dev environment side and the CI side in case of the Terraform changes. yarn reset-local-cloud yarn run-unit-tests *service* But it doesn't make sense as long as we always need to run both commands every time on the CI-side to make the Terraform schema up-to-date in the localstack (and probably on the local dev environment side too). Comment Actions Curious for @varun's perspective on how we can avoid tests starting to fail as a result of Terraform changes Comment Actions I'm confused why unit tests require interfacing with cloud services in the first place. That sounds more like an integration test to me. Comment Actions In some unit tests for blob, backup, tunnelbroker, we are interacting with the local instances of dynamoDB and s3 from localstack and RabbitMQ docker images. Comment Actions Don't think it should count as a unit test if it requires a service to be running locally. Generally a unit test is testing a class or function or file of code, and if there's any external dependencies those will usually get "mocked". Ignoring the naming question, though... it seems like the issue here is that these tests rely on localstack to be running, but if something changes with the Terraform config then localstack may be running based on outdated configuration. Curious to learn more about the relationship between localstack and Terraform. Does Terraform directly configure localstack? Comment Actions Can we just abandon this? It's 3 vs 1 in the advantage of doing so. It should be natural to just throw away the idea instead of still trying to prove that it's good even if there's 2vs1. The arguments are pretty clear, I perceive continuing the discussion here to be inefficient and pointless. If that was up to me, I'd abandon. I'm just going to resign, you can do what you feel is the best. Comment Actions Just to be clear, I'm not saying we need to abandon... just that it doesn't really count as a "unit test" if you need external dependencies running. It's more of an integration test, in my opinion. My concern is just about naming. @karol, are your concerns about naming or about something else? Comment Actions Yes, I checked and these tests are not really unit. We could just name them gtests or something. They, in fact, use s3 and dynamoDB. Comment Actions Cool, so let's maybe make a task to rename it, or alternately we could rename in this diff Comment Actions It seems like the agreement is to keep this but with a different name, because these are not unit tests. That would mean that we should no longer run tests that require the cloud as a part of run_unit_tests.sh. Comment Actions I'll do this in the current diff with the title and description changes to preserve our discussion. Comment Actions Seem so, I'll do this in the current diff with the appropriate changes in a title and description to keep the discussion. Comment Actions In D5129 @jon filtered out the non-unit tests in the Tunnelbroker unit test suite. As discussed here, unit tests should never have a dependency on a service... that's kind of the definition of "unit tests", in fact. Previously we've discussed simply renaming the Tunnelbroker unit tests to integration tests. Rather than that, I think we should separate out the actual unit tests (and keep them defined as "unit tests") from the tests that have a dependency on a service, which we can call "integration tests". (Another option would be "mock" the dependency on the service, but I don't have enough context on the tests to know what the right path forward is.) @max, can you create a task to track this work? Comment Actions I created https://linear.app/comm/issue/ENG-1814, feel free to edit it doesn't align with intention. |