kblincoe / VisualGit_SE701_2019_4

1 stars 0 forks source link

Jest Test Suite Setup, module upgrades and tests for AuthenticationService/Component #122

Closed r4inee closed 5 years ago

r4inee commented 5 years ago

Resolves #120, resolves #106.

Partially contribute to #46.

r4inee commented 5 years ago

Not sure if the commented code should be kept, probably delete them if they are kept for documentation purposes as we can just refer to earlier commits.

Also as we've discussed earlier the unit test cases are to be placed with the file it tests. This is so it's easier to find what tests has been written, and to avoid long "../../" in imports.

Good job on the test cases and all though, you've set up an example of how tests should be written for everyone working on the project. And thanks for upgrading & resolving the Jest issue I was not aware of.

🚀 🚀 🚀

Yeah, I got some conflicts now after #121 got merged in. I think it's best to leave the code in comments for functionality that hasn't been migrated to services. Most of the functions do not have commit history since it was already in the repository before we started. This way people will also know about this (via conflicts) when they rebase.

qw commented 5 years ago

Not sure if the commented code should be kept, probably delete them if they are kept for documentation purposes as we can just refer to earlier commits. Also as we've discussed earlier the unit test cases are to be placed with the file it tests. This is so it's easier to find what tests has been written, and to avoid long "../../" in imports. Good job on the test cases and all though, you've set up an example of how tests should be written for everyone working on the project. And thanks for upgrading & resolving the Jest issue I was not aware of. 🚀 🚀 🚀

Yeah, I got some conflicts now after #121 got merged in. I think it's best to leave the code in comments for functionality that hasn't been migrated to services. Most of the functions do not have commit history since it was already in the repository before we started. This way people will also know about this (via conflicts) when they rebase.

Ok, after fixing the nit picks it'll be LGTM.

r4inee commented 5 years ago

Closing PR, separating upgrading angular and typescript with adding tests