Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: server test with docker CI #36

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

kartikp36
Copy link
Member

No description provided.

@kartikp36 kartikp36 requested a review from harsilspatel August 16, 2021 20:00
@kartikp36 kartikp36 mentioned this pull request Aug 16, 2021
uses: actions/checkout@v2

- name: Login to DockerHub
run: docker login -u $DOCKER_USER -p $DOCKER_PASS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we logging to DockerHub again? Can we not use the Dockerfile from this repo?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to login for Dockerfile to PULL images.

WORKDIR /usr/app
COPY ./types.d.ts ../
COPY . .
RUN npm install typescript -g && npm install
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can you not notice extra spaces? Also why are we installing typescript separately, it's already a dev dependency

Suggested change
RUN npm install typescript -g && npm install
RUN npm install typescript -g && npm install

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are installing typescript globally for using command tsc

postgres:
image: postgres
ports:
- "35432:5432"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

preset: "ts-jest",
testEnvironment: "node",
moduleNameMapper: {
"@exmpl/(.*)": "<rootDir>/src/$1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is being used anywhere

server/service.ts Outdated Show resolved Hide resolved
const response = await request(app).get("/notes/99");
test("GET request by a random id to get 404 response", async () => {
const response = await request(app).get(
"/notes/c61f1fc9-e0d2-43ce-a6c1-b5436cdebe7d"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is c61f1fc9-e0d2-43ce-a6c1-b5436cdebe7d coming from

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just an example in uuid format as 99 is invalid now.

@@ -0,0 +1,10 @@
interface Note {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I despise duplicated code, don't we already have types?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've temporarily added it for ease for work.

@kartikp36 kartikp36 requested a review from harsilspatel August 25, 2021 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants