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

CachingUtilitiesTest.py: Add test for local build #5216

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

harshhx17
Copy link
Contributor

@harshhx17 harshhx17 commented Feb 27, 2018

This adds test to CachingUtilitiesTest.py for the code in
CachingUtilities.py which runs when coala is run for the first
time on the project

Fixes #5206

For short term contributors: we understand that getting your commits well
defined like we require is a hard task and takes some learning. If you
look to help without wanting to contribute long term there's no need
for you to learn this. Just drop us a message and we'll take care of brushing
up your stuff for merge!

Checklist

  • I read the commit guidelines and I've followed
    them.
  • I ran coala over my code locally. (All commits have to pass
    individually.
    It is not sufficient to have "fixup commits" on your PR,
    our bot will still report the issues for the previous commit.) You will
    likely receive a lot of bot comments and build failures if coala does not
    pass on every single commit!

After you submit your pull request, DO NOT click the 'Update Branch' button.
When asked for a rebase, consult coala.io/rebase
instead.

Please consider helping us by reviewing other peoples pull requests as well:

The more you review, the more your score will grow at coala.io and we will
review your PRs faster!

@harshhx17
Copy link
Contributor Author

Here are the new local build logs
https://gist.github.com/harshhx17/eef0dc45b04dec9a3bca931e8f97e72c

Copy link
Member

@alphadose alphadose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your commit message needs work.
This adds test... ->
Add test for CachingUtilities when coala is run for the first time on the project.

Add test for CachingUtilities when coala is run for the first
time on the project.

Fixes coala#5206
@harshhx17
Copy link
Contributor Author

@Makman2 can you please review it 😄

with patch('coalib.misc.CachingUtilities.pickle_load') as \
mocked_pickle_load:
mocked_pickle_load.return_value = {}
self.assertFalse(settings_changed(self.log_printer, settings_hash))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not solve the problem. It just patches the symptom.

To properly solve this, create a tearDown that deletes all side effects from the tests after the tests are done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By this, I mean to say remove all keys related to tests from the pickle after the tests are done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replying to ^ , this isnt about what you have done or not done.

@adtac is asking you to re-create the scenario which this if is supposed to address.

Your code does force the use of this if branch, which does achieve 100% coverage, but it doesnt reproduce the scenario, so the objective of 100% coverage isnt obtained.

The line settings_hash_db = pickle_load(None, 'settings_hash_db', {}) will already return {}, so you dont need to mock pickle_load. You could mock something quite a bit deeper to cause {} to be returned. That may not make @adtac happy, but at least you are more closely recreating the scenario.

@adtac is saying you should use setUp and tearDown to create the scenario, e.g. create a backup of the setting cache, remove the settings for this directory, and then restore the settings again after the test is completed.

As it is an invasive test process, I suggest putting it into its own test class.

@harshhx17
Copy link
Contributor Author

I will look into it 😄

@harshhx17
Copy link
Contributor Author

@adtac
imo, Since I have not created any class variable, I do not think the variables related to the pickle tests will exist outside the scope of the patch call...
However, if i had used a patcher(and mocked CachingUtilities.pickle_load as self.mocked_pickle_load) in the setup(which would mock the pickle_loadd to all the tests), I would need to add that to the teardown so that it doesnot effect other tests....[which i have not done as it is not required by all the tests]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Tests fail to reach 100% code coverage locally, though CI build passes.
5 participants