Increase unit test code coverage to 90% about cccatalog-frontend

[TheCelestial25]

I would like to work on this if permitted. Any directions?

[kgodey]

Go ahead @TheCelestial25. There's a similar issue here: #232, just post the areas you're working on either here or on that issue so that no one else duplicates that work.

[TheCelestial25]

Ma'am, I went through the repository and found that many tests are fixed in a recent PR. I will try to write tests for SearchGridForm, SearchGridFilter and PhotoDetailsPage which are not yet present. Thanks!

[kgodey]

Sounds good!

[TheCelestial25]

@kgodey Ma'am I tried writing tests for two components which are remaining but there was an issue while setting up the environment so it was cumbersome to test those changes online although I tried by force pushing those changes in my PR. I will continue to work on this but I request you to remove the "in progress" label so that it would be open and other interested people can work and the results would come faster. Thought informing you will be good as not doing so would have wasted precious time of other developers. Thanks!
And I came across an issue where some errors in test file can make the TravisCI tests run for infinitely long time.

[kgodey]

Thanks for the update! Please open a new issue describing the TravisCI behavior; we can investigate and see if it's a bug in our code or the expected result.

[TheCelestial25]

@kgodey Ma'am this time while I tried to replicate the behavior, the tests passed instead of stalling. Maybe there was an issue with TravisCI when I first encountered the issue. Also, ma'am I wanted to ask - Is it ideal behavior that TravisCi runs the unit tests written in unmerged PRs (because any unit test until approved by you or other mentors has chances for error) ? If it is not, I would open an issue about it after your approval. Thanks!

[kgodey]

Yes, TravisCI should be running on unmerged PRs – we do not want to merge PRs unless all the tests are passing, it helps us verify that nothing broke because of the changes in that PR. If there are failing tests, they need to be fixed before we can merge the PR.

Also there is no need to call me ma'am. :)

[TheCelestial25]

Thanks, I was talking about running the TravisCI tests by including the unit test changes in that PR. If someone adds a new unit test, should that be run before merging?
Regarding calling you ma'am, I respect you a lot. You've been very helpful by promptly addressing issues and PRs. It would be a privilege to work with mentors like you if I get selected.

[kgodey]

Thanks, for the kind words!

New unit tests should absolutely be run before merging – the master branch of the repository is supposed to always be a stable version that can be deployed to production [1], so only code that is fully working should be merged in. Ideally every pull request will include newly written tests to cover the change in functionality and those new tests need to pass in order for us to be able to rely on them.

[1] We use a simpler version of the popular Git flow branching model (the main changes are that we do not currently maintain a develop branch or use release branches because our team is pretty small and it would be too much overhead).

[TheCelestial25]

Thanks for the information! Fingers crossed, I look forward to learn a lot here. :)
Regards to the overhead you mentioned. Does the absence of a bot make things tedious for the mentors here, like assigning issues to people and adding labels?

[kgodey]

I think our team is small enough that there's not a lot of tedious work associated with those kinds of tasks. We do actually have a couple of bots/scripts doing some of that work.

[TheCelestial25]

Thanks for the reply!

[kgodey]

Closing this in favor of #232.