search

mitodl / rapid-response-xblock

a django app plug-in for edx-platform
BSD 3-Clause "New" or "Revised" License
0 stars 2 forks source link

Add tests for logger.py #96

Closed noisecapella closed 3 years ago

noisecapella commented 3 years ago

It would be useful to have a test suite for this file so we can test that different kinds of events work correctly

asadiqbal08 commented 3 years ago

Ok guys, so I am writing down the tests to support logger.py file as mentioned in the ticket but in middle of my development, I noticed, this file is already covered by tests suits of test_event.py file. Can anyone confirm the scope/purpose of this issue then ?

cc: @pdpinch

pdpinch commented 3 years ago

🤦🏻‍♂️ @noisecapella what do you think we need to do here?

asadiqbal08 commented 3 years ago

Any update over it or should I close this one ?

noisecapella commented 3 years ago

Sorry, I missed that there was test_events.py already. However there are a few missing tests. When I wrote that I was looking at this PR https://github.com/mitodl/rapid-response-xblock/pull/93 which changes the > to a !=. In general when we make a fix like this we should always add a test to verify that fix, and it was missing from that PR.

We also should add a test for this line so that we can test that misformed events don't cause exceptions: https://codecov.io/gh/mitodl/rapid-response-xblock/src/master/rapid_response_xblock/logger.py#L54

Related, our code coverage upload seems to be broken since there are no new updates on codecov.io for the last 4 months. We may have forgotten to do that when we switched to github actions. I'll make an issue for that.

asadiqbal08 commented 3 years ago

@noisecapella test for https://github.com/mitodl/rapid-response-xblock/pull/93 is already covered by test_events.py file however I added a test for missing coverage area as you reported above.

Here is the PR

noisecapella commented 3 years ago

@asadiqbal08 I should not be able to revert #93 and have all the tests pass. Whatever was being fixed by that PR is not being tested. Can you add a test such that it's not just testing that it returns None when len(event_submissions) is 0 but also when len(event_submissions) is 2 or higher?

asadiqbal08 commented 3 years ago

Can you add a test such that it's not just testing that it returns None when len(event_submissions) is 0 but also when len(event_submissions) is 2 or higher?

@noisecapella , It still seems to me this scenario is already covered. I want to know, did you get chance to look around the test_events.py file. https://github.com/mitodl/rapid-response-xblock/blob/master/tests/test_events.py#L204-L212

noisecapella commented 3 years ago

I got the two mixed up. We are missing a test where len(event_submissions) is 0. Can you add that test?

In general, if there is a PR which fixes a bug like #93, we must add a test for it. I can revert the change from #93 and run all tests and not have any tests fail. Do you understand what I'm getting at?

arslanashraf7 commented 3 years ago

@asadiqbal08 I think this can be closed. Feel free to reopen if you think otherwise.