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

wip(mongo-sample): rearrange sample - add in-memory for e2e, mockingbird and dto's #1221

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

omermorad
Copy link

@omermorad omermorad commented Feb 13, 2021

The purpose of this pull request is to put some order in the "cat" entity.
I reorganized the entities: interface, DTO and turned the Mongoose Schema into a class (using nestjs mongoose decorators).

Other than that I plan to add an e2e test containing a mongo-in-memory connection and use the Mockingbird library to create some fixtures.

@omermorad omermorad changed the title wip(mongo-sample): rearrange entity - interface, dto and schema wip(mongo-sample): rearrange sample - add in-memory for e2e, mockingbird and dto's Feb 13, 2021
# Conflicts:
#	apps/mongo-sample/src/cat/service/cat.service.spec.ts
@omermorad omermorad marked this pull request as draft February 13, 2021 13:31
@jmcdo29
Copy link
Owner

jmcdo29 commented Feb 13, 2021

Lots of changes I'm seeing! Most of them good. Can we stay away from the feature/service/feature.service directory structure though, and keep it as feature/feature.service the way it was?

@omermorad
Copy link
Author

Haha yes, a lot of changes.

Thanks! And yes, sure, I will change the structure accordingly :)

const moduleRef: TestingModule = await Test.createTestingModule({
imports: [AppModule],
})
.overrideProvider(MongooseModuleConfigService)
Copy link
Author

Choose a reason for hiding this comment

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

@jmcdo29, what do you think about the in-memory practice?

Copy link
Owner

Choose a reason for hiding this comment

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

I think it looks like it could work well. We should make sure to be explicit in that it is an in-memory mock. Maybe naming the file like cat.in-memory.e2e-spec.ts?

Copy link
Author

Choose a reason for hiding this comment

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

I think the name should not indicate the kind of mock/DB you're using in your test - but I can still change it if you like to.
Maybe add some comments at the beginning of the file?

Copy link
Owner

Choose a reason for hiding this comment

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

Only reason to mention the naming thing here is that this is a repo of samples, so I want people to be sure they know what they're looking at. Normally, I agree, the naming shouldn't matter so much, but as it's all example based, I'd like it to

@omermorad omermorad marked this pull request as ready for review February 13, 2021 19:57
Comment on lines +23 to +60
const mockCat: (
name?: string,
id?: string,
age?: number,
breed?: string,
) => Cat = (
name = 'Ventus',
id = 'a uuid',
age = 4,
breed = 'Russian Blue',
): Cat => ({
name,
id,
age,
breed,
});
) => {
return {
name,
id,
age,
breed,
};
};

// still lazy, but this time using an object instead of multiple parameters
const mockCatDoc = (mock?: Partial<Cat>): Partial<CatDoc> => ({
name: mock?.name || 'Ventus',
_id: mock?.id || 'a uuid',
age: mock?.age || 4,
breed: mock?.breed || 'Russian Blue',
});
const mockCatDoc: (mock?: {
name?: string;
id?: string;
breed?: string;
age?: number;
}) => Partial<CatDocument> = (mock?: {
name: string;
id: string;
age: number;
breed: string;
}) => {
return {
name: (mock && mock.name) || 'Ventus',
_id: (mock && mock.id) || 'a uuid',
age: (mock && mock.age) || 4,
breed: (mock && mock.breed) || 'Russian Blue',
};
};
Copy link
Owner

Choose a reason for hiding this comment

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

Can we revert this change back to the shorter version?

Copy link
Author

Choose a reason for hiding this comment

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

What do you mean by 'shorter version'?

Copy link
Owner

Choose a reason for hiding this comment

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

const mockCatDoc = (mock?: Partial<Cat>): Partial<CatDoc> => ({
  name: mock?.name || 'Ventus',
  _id: mock?.id || 'a uuid',
  age: mock?.age || 4,
  breed: mock?.breed || 'Russian Blue',
});

Instead of

const mockCatDoc: (mock?: {
  name?: string;
  id?: string;
  breed?: string;
  age?: number;
}) => Partial<CatDocument> = (mock?: {
  name: string;
  id: string;
  age: number;
  breed: string;
}) => {
  return {
    name: (mock && mock.name) || 'Ventus',
    _id: (mock && mock.id) || 'a uuid',
    age: (mock && mock.age) || 4,
    breed: (mock && mock.breed) || 'Russian Blue',
  };
};

@@ -1,9 +1,9 @@
import { createMock } from '@golevelup/nestjs-testing';
Copy link
Owner

Choose a reason for hiding this comment

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

Move this file back to mongo-sample/src/cat/cat.controller.spec.ts

@@ -8,9 +8,9 @@ import {
Post,
Copy link
Owner

Choose a reason for hiding this comment

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

move this back to mongo-sample/src/cat/cat.controller.ts

@@ -0,0 +1,5 @@
import { Cat } from '../interfaces/cat.interface';
Copy link
Owner

Choose a reason for hiding this comment

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

Move this to mongo-sample/src/cat/cat.dto.ts

@@ -0,0 +1,17 @@
import { Document } from 'mongoose';
Copy link
Owner

Choose a reason for hiding this comment

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

Move this back to mongo-sample/src/cat/cat.document.ts

@omermorad
Copy link
Author

@jmcdo29 I'm still working on the example, I had a rough week :/
Also - I'm planning to change the Mockingbird package API, can we wait with this a bit? I really want to enrich this example!

@jmcdo29
Copy link
Owner

jmcdo29 commented Feb 19, 2021

@jmcdo29 I'm still working on the example, I had a rough week :/
Also - I'm planning to change the Mockingbird package API, can we wait with this a bit? I really want to enrich this example!

Sure. Feel free to keep this open and work when you can or just close it out and create a new PR.

@omermorad
Copy link
Author

I will keep this one open 🙏🏼
I will update it soon as I can

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.

3 participants