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.

Sign up for GitHub

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

Jump to bottom

add unit tests #19

Open
1 of 2 tasks
abhijithvijayan opened this issue Oct 12, 2019 · 32 comments · Fixed by #22
Open
1 of 2 tasks

add unit tests #19

abhijithvijayan opened this issue Oct 12, 2019 · 32 comments · Fixed by #22
Labels
good first issue Issue that doesn't require previous experience with this project. help wanted Issue with a clear description that the community can help with.

Comments

@abhijithvijayan
Copy link
Owner

abhijithvijayan commented Oct 12, 2019 •
edited

Checklist

  • Initial barebone
  • Add unit tests for the commands offered by the cli
@abhijithvijayan abhijithvijayan added enhancement help wanted Issue with a clear description that the community can help with. labels Oct 12, 2019
@JacobMGEvans
Copy link
Contributor

Could I get assigned to this?

@abhijithvijayan
Copy link
Owner Author

@JacobMGEvans How is that PR coming ?

@JacobMGEvans
Copy link
Contributor

JacobMGEvans commented Oct 12, 2019 via email

@JacobMGEvans JacobMGEvans mentioned this issue Oct 12, 2019
Merged
@JacobMGEvans
Copy link
Contributor

I did add exports to a lot of the functions in the file, but those can easily be removed when determined which ones are not needed for isolated testing.

I was also trying to get a feel for the codebase/the flow of data and how to approach testing it.

@abhijithvijayan
Copy link
Owner Author

@JacobMGEvans I think a lot of code refactoring is needed here and there in the source code. This was all written very soon so didn't get time to refactor.

@JacobMGEvans
Copy link
Contributor

Well, tests will definitely help when you go to refactor because you can just run watch on them and make sure the tests don't break :)

I try to write tests so the code implementation doesn't matter i.e. refactors dont break them... the only thing that matters is the input => output and behaviors remain the same/consistent.

@abhijithvijayan
Copy link
Owner Author

abhijithvijayan commented Oct 14, 2019 •
edited

@JacobMGEvans You want me to assign this to someone else?

Refactor seems to be quite needed as CLI hit 2700+ weekly downloads. 😀

@JacobMGEvans
Copy link
Contributor

Wow! Congratulations, definitely a good idea to refactor for performance then!

You can assign other people, I probably will still write tests soon regardless. I am just finishing up with another issue from a different OSS and a few from my own side project.

@abhijithvijayan abhijithvijayan added enhancement good first issue Issue that doesn't require previous experience with this project. help wanted Issue with a clear description that the community can help with. and removed enhancement help wanted Issue with a clear description that the community can help with. labels Oct 14, 2019
@JacobMGEvans
Copy link
Contributor

JacobMGEvans commented Oct 14, 2019 •
edited

Also I really like your project and how you have communicated, it's been a pleasant experience. So I certainly would like to assist 😄

@abhijithvijayan
Copy link
Owner Author

@JacobMGEvans Thanks a lot for that.

It's not everyday someone helps in such tiny OSS projects. Glad that you are ready to contribute. Appreciate it.

@JacobMGEvans
Copy link
Contributor

Last night I started focusing on the unit tests, I am having issues importing the functions and statements for some reason... Might need to spend a little more time on the jest configuration, not sure what's the exact problem yet.

@abhijithvijayan
Copy link
Owner Author

@JacobMGEvans Told ya, it's messed up.

@JacobMGEvans
Copy link
Contributor

lmao, I am certain I can figure it out I just wanted to keep you updated 😆

@JacobMGEvans
Copy link
Contributor

JacobMGEvans commented Oct 16, 2019 •
edited

@abhijithvijayan
Alright, I got this mostly figure out it was not configuration issues. The fact you were using a module.exports on an anonymous async function was making is impossible to test... I have some solutions,

  1. firstly I am going to make the test work with the code as-is then make the PR

  2. I am going to refactor it to use ES6 modules import/export which export default should would the same as how you have module.exports with the testing in place we can refactor with confidence it will keep working as expected.

  3. Once the import/export is in place testing specific functions will be far easier and refactoring the code base will be far easier.

  4. Integration and E2E(likely not needed) will be the next endeavor

@abhijithvijayan
Copy link
Owner Author

@JacobMGEvans I am open to any modifications as long as the project works and code quality is improved.

@JacobMGEvans
Copy link
Contributor

@abhijithvijayan Interesting enough if I try to pass false into the options that are primitives is throws an error, if I pass true it works fine... I might open a bug issue a little bit

@JacobMGEvans
Copy link
Contributor

Would you be able to give me some mock or actual payloads and responses? I would rather not go through the whole process if you have anything like that leftover from developing it.

@abhijithvijayan
Copy link
Owner Author

abhijithvijayan commented Oct 16, 2019 •
edited

@JacobMGEvans I have fixed the export issue and also added a sample mock payload to the test file.

Now you can continue adding tests.

@abhijithvijayan abhijithvijayan mentioned this issue Oct 16, 2019
Closed
@JacobMGEvans
Copy link
Contributor

JacobMGEvans commented Oct 17, 2019 •
edited

Huh, it was working fine when I tested it locally... I use babel-node though 😬 lol

Glad you got it fixed though Ill check out the changes and continue with testing.

@JacobMGEvans
Copy link
Contributor

Someone also uncommented out tests that I had TODOs on because they were incomplete and a comment was added that "it doesnt return anything" which is why there was a TODO there to finish the test...

@abhijithvijayan
Copy link
Owner Author

@JacobMGEvans I was trying out the error fixes. Feel free to work on it.

@abhijithvijayan
Copy link
Owner Author

@JacobMGEvans Is babel config and its packages needed to run the tests?

@JacobMGEvans
Copy link
Contributor

JacobMGEvans commented Oct 18, 2019 via email

@JacobMGEvans
Copy link
Contributor

https://jestjs.io/docs/en/getting-started#using-babel

@abhijithvijayan
Copy link
Owner Author

@JacobMGEvans The tests are almost complete right!

@JacobMGEvans
Copy link
Contributor

JacobMGEvans commented Oct 20, 2019 via email

@abhijithvijayan
Copy link
Owner Author

@JacobMGEvans not really.

CLI creates README.md, and local file is readme.md

If you have tried the cli within the folder, you can see it doesn't affect the original readme.md file

@JacobMGEvans
Copy link
Contributor

I wasn't asking a question I was stating what happens when you run the function in the Jest test, also it is not cap sensitive, Node just writes to the readme.md, like I said.

@abhijithvijayan
Copy link
Owner Author

I thought since the command Node cli.js -u "USERNAME" creates README.md along with readme.md, it wouldnt be a problem too, while testing.

Maybe renaming current readme.md to just readme might help right!

@JacobMGEvans
Copy link
Contributor

JacobMGEvans commented Oct 21, 2019 via email

@abhijithvijayan abhijithvijayan pinned this issue Oct 22, 2019
@abhijithvijayan
Copy link
Owner Author

@JacobMGEvans A lot has changes since the last test added.

I have refactored the code a lot and it seems to be stable now. All the test suites are still working as expected.

Are you currently on other projects now for not be able to work on this issue anymore?

@JacobMGEvans
Copy link
Contributor

JacobMGEvans commented Oct 29, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Issue that doesn't require previous experience with this project. help wanted Issue with a clear description that the community can help with.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Test Scaffolding/Infrastructure in place JacobMGEvans/stargazed
2 participants
@JacobMGEvans @abhijithvijayan