refactor(scripts): add comments, remove duplicates, augment jest #263

jpetto · 2021-11-02T22:51:59Z

Goal

add comments and clean up some scripts. also add extra jest config options.

removed scripts directory - why do we need it?
added comments to circleci/config.yml & related sh scripts
added extra jest config

I'd love feedback/perspectives on:

why we had both scripts and scripts.example directories in .circleci. these were kind of doing the same thing, though with small inconsistencies. seems confusing to me to keep both.

Implementation Decisions

added a new jest.setup.js file to help with (potential) local (outside of container) integration tests.

- removed scripts directory - why do we need it? - added comments to circleci/config.yml & related sh scripts - added extra jest config

kkyeboah · 2021-11-02T23:02:34Z

.circleci/scripts.example/setup_hosts.sh

 echo "Adding service hosts records"

 # These should match the services you are creating in your docker-compose file
+# This is required for containers to talk to each other


Suggested change

# This is required for containers to talk to each other

# This is required for CircleCI to talk to containers if you export environment variables to the shell that change the host of running containers from localhost to something else like `mysql` and `localstack`

can you explain this a bit more? in this instance does "CircleCI" mean a server (not a container) within circle's infrastructure that talks to our specified containers?

@jpetto From my understanding, when a job is triggered on Circle CI, it provisions a VM (think your local machine) and runs the containers, exposing them on localhost.

Before we run integration tests we export the environment variables that our application depends on to the shell. This means that when tests are running and using configurations from our config/index.ts file (which pulls from these env vars), what it reads are the env var definitions for the docker container. CircleCI does not know what mysql means, so we tell CircleCI that when you see mysql, please look at it as localhost or 127.0.0.1.

Now you may be asking, just as I'm asking myself now, that with projects like prospect-api that are set up to run outside the container without needing any of these env vars, why do we export anything to the shell? I think the answer is that we don't need to and CIrcleCI should just work. I will confirm this after these short messages :)

Let me know if a live chat might help a bit more here.

kkyeboah · 2021-11-02T23:03:36Z

jest.config.js

@@ -3,4 +3,6 @@ module.exports = {
  testEnvironment: 'node',
  testMatch: ['**/?(*.)+(spec|integration).ts'],
  testPathIgnorePatterns: ['/dist/'],
+  // TODO: remove the following if you don't have any localstack integration tests


Suggested change

// TODO: remove the following if you don't have any localstack integration tests

// TODO: remove the following if you don't have any setup you would like to do before your tests run. Ex. setting a different endpoint for localstack services for integration tests

i was under the impression that if localstack was in use, an endpoint had to be defined - e.g. http://localstack:4566. is that not the case?

My comment is more to the point that this config allows including a setup file that is not just for setting up env vars for localstack, you can pretty much do anything in the setup file like, for example, create a tmp directory for all tests to output something to.

kkyeboah · 2021-11-02T23:05:43Z

jest.setup.js

@@ -0,0 +1,4 @@
+// your AWS_ENDPOINT values should be set in .docker/local.env
+// this file is to provide a value when running tests locally outside of the container
+// TODO: if you don't have any localstack integration tests, you can delete this file


Suggested change

// TODO: if you don't have any localstack integration tests, you can delete this file

// TODO: if you don't have any localstack integration tests, you can delete this file

// Generally, if you don't have any setup you would like to do before tests, you can delete this entire file

kkyeboah · 2021-11-02T23:07:33Z

src/config/index.ts

+  // TODO: if you aren't using localstack, you can get rid of the `aws` section
+  aws: {
+    localEndpoint: process.env.AWS_ENDPOINT,
+  }


Suggested change

}

},

kkyeboah

could we also copy the content of scripts to scripts.example?

jpetto · 2021-11-03T14:40:41Z

could we also copy the content of scripts to scripts.example?

this diff is weird - all the files in scripts.example have content for me locally. i just pulled this PR down (git fetch origin pull/263/head:pr/263 && git checkout pr/263) and i see they all have content. idk why it's showing them as empty in this diff. can you pull this locally and verify?

kkyeboah · 2021-11-03T18:54:49Z

@jpetto confirmed that scripts.example files have content that is not showing up in this PR.

refactor(scripts): add comments, remove duplicates, augment jest

8e16493

- removed scripts directory - why do we need it? - added comments to circleci/config.yml & related sh scripts - added extra jest config

jpetto requested a review from a team as a code owner November 2, 2021 22:51

jpetto requested review from kkyeboah and Herraj November 2, 2021 22:51

kkyeboah reviewed Nov 2, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(scripts): add comments, remove duplicates, augment jest #263

refactor(scripts): add comments, remove duplicates, augment jest #263

jpetto commented Nov 2, 2021

kkyeboah Nov 2, 2021 •

edited

jpetto Nov 3, 2021

kkyeboah Nov 3, 2021

kkyeboah Nov 2, 2021

jpetto Nov 3, 2021

kkyeboah Nov 3, 2021 •

edited

kkyeboah Nov 2, 2021

kkyeboah Nov 2, 2021

kkyeboah left a comment

jpetto commented Nov 3, 2021

kkyeboah commented Nov 3, 2021 •

edited

	# This is required for containers to talk to each other
	# This is required for CircleCI to talk to containers if you export environment variables to the shell that change the host of running containers from localhost to something else like `mysql` and `localstack`

	// TODO: remove the following if you don't have any localstack integration tests
	// TODO: remove the following if you don't have any setup you would like to do before your tests run. Ex. setting a different endpoint for localstack services for integration tests

	// TODO: if you don't have any localstack integration tests, you can delete this file
	// TODO: if you don't have any localstack integration tests, you can delete this file
	// Generally, if you don't have any setup you would like to do before tests, you can delete this entire file

refactor(scripts): add comments, remove duplicates, augment jest #263

Are you sure you want to change the base?

refactor(scripts): add comments, remove duplicates, augment jest #263

Conversation

jpetto commented Nov 2, 2021

Goal

I'd love feedback/perspectives on:

Implementation Decisions

kkyeboah Nov 2, 2021 • edited

Choose a reason for hiding this comment

jpetto Nov 3, 2021

Choose a reason for hiding this comment

kkyeboah Nov 3, 2021

Choose a reason for hiding this comment

kkyeboah Nov 2, 2021

Choose a reason for hiding this comment

jpetto Nov 3, 2021

Choose a reason for hiding this comment

kkyeboah Nov 3, 2021 • edited

Choose a reason for hiding this comment

kkyeboah Nov 2, 2021

Choose a reason for hiding this comment

kkyeboah Nov 2, 2021

Choose a reason for hiding this comment

kkyeboah left a comment

Choose a reason for hiding this comment

jpetto commented Nov 3, 2021

kkyeboah commented Nov 3, 2021 • edited

kkyeboah Nov 2, 2021 •

edited

kkyeboah Nov 3, 2021 •

edited

kkyeboah commented Nov 3, 2021 •

edited