-
-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
test(sample-07): added unit tests #8044
test(sample-07): added unit tests #8044
Conversation
Pull Request Test Coverage Report for Build 1e576ed5-2b5b-4dd5-a96a-d75a1b2b2cd7
💛 - Coveralls |
Conflicts resolved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rest of the tests look good, just remove the one I mentioned or change it so it's mocking the sequelize functionality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few more changes I noticed
Co-authored-by: Jay McDoniel <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -13,6 +14,7 @@ import { UsersModule } from './users/users.module'; | |||
database: 'test', | |||
autoLoadModels: true, | |||
synchronize: true, | |||
models: [User], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed? (and how can we avoid making this change)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I followed the directions in the documentation: https://docs.nestjs.com/techniques/database#models ,
by adding the models in SequelizeModule.forRoot({...})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be required when autoLoadModels
is set to true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't want to be wrong, but in sequelize version 5, registering the model in SequelizeModule.forRoot ({...})
was an alternative to extending the Model class, like this example export class Post extends Model<Post> {.... }
.
Now from version 6, under the hood of sequelize something seems to have changed, so as you said autoLoadModels: true
automatically loads the Modules if set to true
, similar to TypeORM, now I tried to remove models:[]
and it seems to work.
Maybe we should update the documents for this, what do you think?
Then I wanted to ask you but is the models:[]
setting still useful to use? maybe you could remove it as a setting?
Co-authored-by: Kamil Mysliwiec <[email protected]>
Co-authored-by: Nidin Pereira <[email protected]>
Conflicts resolved in |
Conflicts resolved in |
Co-authored-by: Kamil Mysliwiec <[email protected]>
LGTM |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #1539
What is the new behavior?
Added unit tests in sample
07-sequelize
Does this PR introduce a breaking change?
Other information
This PR, added unit tests in sample
07-sequelize