-
Notifications
You must be signed in to change notification settings - Fork 8
feat: mle-ts-ords-backend template #301 #302
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
base: main
Are you sure you want to change the base?
Conversation
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.
great documentation @DimaNike, thanks!
}, | ||
}, | ||
); | ||
const id = result.outBinds.id[0]; |
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 something I didn't notice before, this attribute access is unpredictable, it assumes result.outBinds.id[0]
will exist regardless the response from session.execute
, optional chaining (?
) is a good option here
* @returns {number} The ID of the newly created user. | ||
*/ | ||
export function newUser(name: string): number { | ||
const result = session.execute( |
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.
one thing I can't remember if was asked before: is session.execute
synchronous?
|
||
|
||
-------------------------------------------------------------------------------- | ||
-- PUT ords/userc/users/:id |
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.
looking at the previous code, looks like this should be PUT ords/userc/users/edit/:id
it's missing the edit
it('should create a user (POST)', async () => { | ||
const res = await fetch(`${BASE_URL}/create?name=perry`, { method: 'POST' }); | ||
|
||
expect(res.status).toBe(201); | ||
const data = await res.json(); | ||
expect(data).toHaveProperty('id'); | ||
expect(typeof data.id).toBe('number'); | ||
|
||
createdUserId = data.id; | ||
console.log('New user created with ID:', createdUserId); | ||
}); |
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 tests are dependent on each other, which makes it fragile, although I'm not sure how we could improve this without adding too much code. All I can think of is using a beforeAll
and simplifying this assertion to
it('should create a user (POST)', async () => {
expect(createdUserId).toBeDefined();
expect(typeof createdUserId).toBe('number');
});
and regarding the afterAll
and the delition assertion... we might have to leave it as it already is since we need them to be the last test and would be a bad practice to embed a test in the afterAll
, so a beforeAll
looks to be all we can do. I'd like to know your opinion on this
@@ -0,0 +1,32 @@ | |||
version: '2.4' |
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.
could we use a more recent version instead? this one is from 2022
https://github.com/docker/compose/releases
https://github.com/docker/compose/releases/tag/v2.4.0
[PR] MLE ORDS BACKEND TEMPLATE