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

Inconsistency of package.json test scripts across solution-\d branches #62

Open
bkjoel opened this issue Feb 6, 2021 · 0 comments
Open

Comments

@bkjoel
Copy link

bkjoel commented Feb 6, 2021

Hi there, in branches lesson-1 to lesson-3 and their solution branches the test script commands in package.json (test-routes, test-models, test-controllers...) use npm run test while branches lesson-4, lesson-5 and their solution branches use yarn test.

Shouldn't the test scripts usage of npm or yarn be consistent across branches? should they all use npm since users are more likely to have npm installed than yarn?
If the decision is made to stick with yarn it would be helpful to note in the README that yarn is required and not only suggested.

Also as it currently stands - in the branches that use yarn to run the tests the README suggests using npm run test-* which simply wouldn't work for a user that didn't install yarn (like me 🙃), example attached -
https://github.com/FrontendMasters/api-design-node-v3/tree/lesson-4#controllers

I'd be happy to submit a pull request with updates to the package.json file across branches.

EDIT:
As I keep going through the course I'll add more points on issues I see if that's helpful & fine with you -

In the crud.spec.js tests the test's name is 404 if no doc was found but the actual assertion is expect(status).toBe(400)
In my opinion this should be asserted against 404 since the request wasn't misunderstood by the server but rather the resource wasn't found, would that be correct?

EDIT 2:
On the "Sign up with JWT Solution" video there's a bug with the "creates user and sends new token from user" test if your signup route calls res.status().send() without returning it. In the video Scott just adds the return keyword but I think there's an important lesson to be taught here about async functions returning everything as a promise and an implicit return of undefined.
Since there was no return statement it had returned a promise that resolved immediately to undefined which caused the test block to end & drop the database before the user was fetched. This could be a real gem of an explanation to someone watching the course.

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

No branches or pull requests

1 participant