Add unit tests for the functions in utils.py #63

Shashi456 · 2018-02-17T18:07:28Z

Fixes #6

These are just very basic unit tests , which is my understanding of them. Since we are just checking each module individually and seeing if they do the job they are supposed to do. Also another thing i wanted to ask is how could i go about writing tests for functions which had no parameters, should i just check the existence of what has been returned? Thanks for your help.

@marco-c I will clean up the code and use tempfile in the upcoming commit, this was just for a basic check to see if you were expecting similar unit tests.

poush · 2018-02-17T18:33:09Z

tests/.pytest_cache/v/cache/lastfailed

@@ -0,0 +1,3 @@
+{
+  "test_utils.py::test_answer": true


Hey, I think this file should be in the .gitignore.

Indeed, and it should also be removed from the PR.

marco-c · 2018-02-17T18:37:17Z

tests/test_utils.py

+from keras.preprocessing.image import load_img, img_to_array
+
+
+def mkdir(dir_name):


The functions that need to be tested should not be here.

marco-c · 2018-02-17T18:37:33Z

Also another thing i wanted to ask is how could i go about writing tests for functions which had no parameters, should i just check the existence of what has been returned?

For functions that require the data directory to exist, don't write any test.
For other functions with no parameters (e.g. get_bugs), just make some basic assertion on the result.

marco-c · 2018-02-17T18:38:21Z

These are just very basic unit tests , which is my understanding of them. Since we are just checking each module individually and seeing if they do the job they are supposed to do.

Yes, this is what we should do.

marco-c · 2018-02-17T18:38:38Z

@marco-c I will clean up the code and use tempfile in the upcoming commit, this was just for a basic check to see if you were expecting similar unit tests.

OK, let me know when the PR is ready and I'll review it.

Shashi456 · 2018-02-18T07:52:34Z

@marco-c So i've written basic tests for 6 functions, just asserting the existence of the objects in case of functions with no specific parameters. For writing a couples_generator, getImageGenerator there is image_test or image_train so i have written no function for the same.

marco-c · 2018-02-18T19:22:51Z

tests/test_utils.py

+def test_mkdir():
+    direc_path = os.path.dirname(os.path.realpath(__file__)) + "test"
+    utils.mkdir(direc_path)
+    assert(os.path.exists(direc_path))


You can assert that this is a directory.
Also, you can test calling it again to check that it doesn't fail when the directory already exists.

marco-c · 2018-02-18T19:23:15Z

tests/test_utils.py

+    assert(os.path.exists(direc_path))
+
+
+label = {}


Not sure you need to define this globally.

marco-c · 2018-02-18T19:23:56Z

tests/test_utils.py

+
+def test_read_labels():
+    labels = utils.read_labels(file_name='labels.csv')
+    assert('labels' in locals() or 'labels' in globals())


This assertion is strange. Just assert that labels is not null and that it is a dictionary.

i was just asserting the existence of the variable

You have defined it at line 15, so it clearly exists 😄

marco-c · 2018-02-18T19:24:19Z

tests/test_utils.py

+def test_write_labels():
+    utils.write_labels(label, file_name='test.csv')
+    fname = os.path.dirname(os.path.realpath(__file__)) + "\\test.csv"
+    assert(os.path.exists(fname))


Also assert that you can use read_labels to read the labels back.

marco-c · 2018-02-18T19:24:33Z

tests/test_utils.py

+    assert(os.path.exists(fname))
+
+
+images = {"Image.png": [23434, 324124]}


This is unused.

marco-c · 2018-02-18T19:25:10Z

tests/test_utils.py

+
+def test_load_image():
+    img = utils.load_image("Image.png")
+    assert(img)


assert that this is a numpy 2x2 array

marco-c · 2018-02-18T19:25:52Z

tests/test_utils.py

+
+def test_get_bugs():
+    Object = utils.get_bugs()
+    assert(Object)


assert that this is a list

marco-c · 2018-02-18T19:27:12Z

tests/test_utils.py

+def test_balance():
+    Sample = [[1, 0, 0], [0, 1, 0], [0, 0, 1]]
+    it = utils.balance(Sample)
+    assert(it)


You can actually assert what balance returns here and not just check that it is not null.

marco-c · 2018-02-18T19:29:13Z

For writing a couples_generator, getImageGenerator there is image_test or image_train so i have written no function for the same.

I'm not sure what you mean here, can you expand?

marco-c · 2018-02-18T19:29:36Z

Nit: Can you put the tests in the same order as the functions of test_utils.py?

marco-c · 2018-02-18T19:29:59Z

Also, don't forget to use tempfile to create the temporary directory where you store the temporary test files.

Shashi456 · 2018-02-19T01:04:15Z

@marco-c I have added temporary directory to the unit tests wherever I am making a directory or a file. I have also made other minor changes you asked me to do. I had two doubts though, What is balance returning? and I was getting a row index out of range error while reading the written labels do you have any idea what could cause it?

marco-c · 2018-02-19T10:26:44Z

tests/test_utils.py

+
+
+def test_get_bugs():
+    Object = utils.get_bugs()


Nit: rename Object to bugs.

marco-c · 2018-02-19T10:30:22Z

tests/test_utils.py

+    d = TemporaryDirectory()
+    direc_path = os.path.join(d.name + "\\test")
+    utils.mkdir(direc_path)
+    assert(os.path.isdir(direc_path))


Second part from #63 (comment)

@marco-c Instead of writing a test for it , would it not make more sense to change the mkdir function to check if a directory with the same name exists and not make it if it does? since the function the doesnt return any error message it is pretty hard to assert on anything . If some error message were returned i could assert on the same .

You don't need to add an assertion, the assertion is implied in the fact that utils.mkdir doesn't throw an exception.

marco-c · 2018-02-19T10:31:01Z

tests/test_utils.py

+
+
+def test_load_image():
+    img = utils.load_image("Image")


This file doesn't exist in the repository, so I'm not sure the test will pass.

@marco-c earlier i had the load_image function in the file and i was using a random dictionary to just check if the code was working, Because an empty file name would result in a parameter not declared error. Should i remove the test ?

You can add a parent_dir argument to load_image, with the default being data_resized.
Then in the test you can create a fake image in the temporary directory, pass the temporary directory as parent_dir to load_image and then assert that the image is loaded as a 2x2 numpy array with the correct dimensions.

marco-c · 2018-02-19T10:33:50Z

What is balance returning?

You can see the function implementation or the callers of the functions to understand. If you can't figure it out, please remove the current test_balance and file a follow-up issue to add a test for balance.

and I was getting a row index out of range error while reading the written labels do you have any idea what could cause it?

I'm not sure, as I haven't seen what you were trying to do. If you can't figure it out, please file a follow-up issue to add the test.

marco-c · 2018-02-19T10:34:13Z

Please also update the .travis.yml file to actually run the tests.

marco-c · 2018-02-19T10:34:31Z

I've noticed there's a nice pytest feature that you can use instead of manually using tempfile: https://docs.pytest.org/en/latest/tmpdir.html.

Shashi456 · 2018-02-19T16:42:54Z

@marco-c i've made all the minor changes you asked me to, i have removed the balance test and will open a pr for the same, because balance seems to be interacting with Couples-generator but we aren't writing tests for functions which need the data directory. One last thing is the error in the Travis-cl build it is giving an image not found error with the Load_image test, while I am able to pass the test successfully on my pc. Any Thoughts?

marco-c · 2018-02-19T16:56:30Z

@marco-c i've made all the minor changes you asked me to

It seems you haven't done #63 (comment) and #63 (comment).

i have removed the balance test and will open a pr for the same, because balance seems to be interacting with Couples-generator but we aren't writing tests for functions which need the data directory.

The balance function is generic, it doesn't require CouplesGenerator to work. Anyway yes, please open an issue to add a test for this function and an issue for each function in utils.py that isn't tested yet.

One last thing is the error in the Travis-cl build it is giving an image not found error with the Load_image test, while I am able to pass the test successfully on my pc.

Doing #63 (comment) will fix the problem.

Shashi456 · 2018-02-19T16:59:21Z

@marco-c i did modify both the load image file with the parent directory and try it with both temporary and an actual directory. I will look further into, thankyou.

marco-c · 2018-02-19T17:02:46Z

@marco-c i did modify both the load image file with the parent directory and try it with both temporary and an actual directory. I will look further into, thankyou.

Also don't download the image from the network, it's not great for unit tests (what if the image is removed from the website? The tests will start failing). Create a fake image using PIL.

marco-c · 2018-02-19T17:34:55Z

tests/test_utils.py

+    os.mkdir('data_resized')
+    fake_img = np.random.rand(30, 30, 3) * 255
+    img = Image.fromarray(fake_img.astype('uint8')).convert('RGBA')
+    img.save(".\data_resized\Image.jpg", "JPEG")


Don't specify the format, it's automatically detected from the file extension.

Review Changes Review Changes Review Changes Review Changes

Shashi456 · 2018-02-19T17:47:52Z

@marco-c i will open the balance PR as soon as this is merged, i have done all changes you've asked me for . Thankyou

marco-c · 2018-02-19T17:52:54Z

@marco-c i will open the balance PR as soon as this is merged, i have done all changes you've asked me for . Thankyou

I still don't see #63 (comment) 😄

Also, in test_load_image you should use a temporary directory and not manually create and delete data_resized, which would mess the working directory of anyone which runs the tests.

marco-c · 2018-02-19T17:53:28Z

tests/test_utils.py

+def test_load_image():
+    os.mkdir('data_resized')
+    fake_img = np.random.rand(30, 30, 3) * 255
+    img = Image.fromarray(fake_img.astype('uint8')).convert('RGBA')


Use Image.new instead of creating a numpy array and then convering it into an image.

marco-c · 2018-02-19T17:53:59Z

tests/test_utils.py

+    d = TemporaryDirectory()
+    file_path = os.path.join(d.name + "\\test.csv")
+    utils.write_labels(label, file_name=file_path)
+    assert(os.path.exists(file_path))


Remember to file an issue about adding a test that writes labels and then reads them back.

marco-c · 2018-02-19T17:55:18Z

tests/test_utils.py

+
+def test_mkdir():
+    d = TemporaryDirectory()
+    direc_path = os.path.join(d.name + "\\test")


You're calling os.path.join with a single string, which is basically useless.
Either do os.path.join(d.name, 'test') or d.name + '/test'.

marco-c · 2018-02-19T17:56:04Z

tests/test_utils.py

+import numpy as np
+from tempfile import TemporaryDirectory
+from autowebcompat import utils
+from PIL import Image


Sort the imports in this way:

Built-in modules;

Modules from third-party libraries;

Our modules.

Shashi456 · 2018-02-19T17:56:39Z

@marco-c do you necessarily want the new pytest feature and not the Temporary Directory () way?

marco-c · 2018-02-19T17:56:50Z

.travis.yml

@@ -6,3 +6,4 @@ install:
  - pip install -r test-requirements.txt
 script:
  - flake8 .
+  - pytest ./tests/test_utils.py


This is going to execute test_utils.py only, please make it run any test that is in the tests directory, so we don't have to update this line the next time we add a test.

Don't forget about this, @Shashi456

marco-c · 2018-02-19T17:58:47Z

@marco-c do you necessarily want the new pytest feature and not the Temporary Directory () way?

The pytest feature is nicer, so I would prefer it. If you can't, please file an issue for someone else to do it.

Shashi456 · 2018-02-19T18:38:29Z

Would you want the PR's to be referenced to this? @marco-c

marco-c · 2018-02-19T18:42:29Z

Would you want the PR's to be referenced to this? @marco-c

Do you mean the issues? Yes

marco-c · 2018-02-19T18:45:24Z

tests/test_utils.py

+
+
+def test_read_labels():
+    labels = utils.read_labels(file_name='labels.csv')


Create a fake labels.csv file in a temporary directory, so you can assert more things about the result.

Actually, don't do it, we'll do it as part of the other test that writes and reads labels.

marco-c · 2018-02-19T18:50:33Z

tests/test_utils.py

+    img = Image.new("RGB", (30, 30))
+    img.save(d.name + "/Image.jpg")
+    img = utils.load_image("Image.jpg", d.name)
+    assert(isinstance(img, np.ndarray))


You can assert the dimensions of the numpy array too.

marco-c

Thanks! I see you have filed follow-up issues. I'm marking them good first bugs for newcomers, but since they are follow-ups to your PR, you can take them if you want (just say so in the issue if you do).

propr · 2018-02-19T20:14:07Z

Please provide your feedback on this pull request here.

Privacy statement: We don't store any personal information such as your email address or name. We ask for GitHub authentication as an anonymous identifier to account for duplicate feedback entries and to see people specific preferences.

poush reviewed Feb 17, 2018

View reviewed changes

marco-c reviewed Feb 17, 2018

View reviewed changes

marco-c reviewed Feb 18, 2018

View reviewed changes

tests/test_utils.py Outdated

assert(os.path.exists(direc_path))

label = {}

Copy link

Owner

marco-c Feb 18, 2018

Choose a reason for hiding this comment

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

Not sure you need to define this globally.

marco-c reviewed Feb 18, 2018

View reviewed changes

tests/test_utils.py Outdated

assert(os.path.exists(fname))

images = {"Image.png": [23434, 324124]}

Copy link

Owner

marco-c Feb 18, 2018

Choose a reason for hiding this comment

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

This is unused.

marco-c reviewed Feb 18, 2018

View reviewed changes

tests/test_utils.py Outdated

def test_get_bugs():

Object = utils.get_bugs()

assert(Object)

Copy link

Owner

marco-c Feb 18, 2018

Choose a reason for hiding this comment

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

assert that this is a list

marco-c reviewed Feb 18, 2018

View reviewed changes

marco-c changed the title ~~[#5] Add unit tests for the functions in utils.py~~ Add unit tests for the functions in utils.py Feb 18, 2018

marco-c reviewed Feb 19, 2018

View reviewed changes

tests/test_utils.py Outdated

def test_get_bugs():

Object = utils.get_bugs()

Copy link

Owner

marco-c Feb 19, 2018

Choose a reason for hiding this comment

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

Nit: rename Object to bugs.

marco-c reviewed Feb 19, 2018

View reviewed changes

Review Changes

907f074

Review Changes Review Changes Review Changes Review Changes

marco-c reviewed Feb 19, 2018

View reviewed changes

Minor Changes

3be5f4d

marco-c reviewed Feb 19, 2018

View reviewed changes

Shashi456 added 2 commits February 20, 2018 00:37

Minor Changes

c8c6027

Updated travis.yml

9099367

marco-c approved these changes Feb 19, 2018

View reviewed changes

marco-c merged commit 7396079 into marco-c:master Feb 19, 2018

marxmit7 pushed a commit to marxmit7/autowebcompat that referenced this pull request Jan 18, 2019

Update pytest from 4.1.0 to 4.1.1 (marco-c#63)

87fe335

		from keras.preprocessing.image import load_img, img_to_array


		def mkdir(dir_name):

		assert(os.path.exists(fname))


		images = {"Image.png": [23434, 324124]}



		def test_read_labels():
		labels = utils.read_labels(file_name='labels.csv')

Add unit tests for the functions in utils.py #63

Add unit tests for the functions in utils.py #63

Conversation

Shashi456 commented Feb 17, 2018 • edited by marco-c

poush Feb 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marco-c commented Feb 17, 2018

marco-c commented Feb 17, 2018

marco-c commented Feb 17, 2018

Shashi456 commented Feb 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marco-c Feb 18, 2018 • edited

Choose a reason for hiding this comment

marco-c commented Feb 18, 2018

marco-c commented Feb 18, 2018

marco-c commented Feb 18, 2018

Shashi456 commented Feb 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marco-c commented Feb 19, 2018

marco-c commented Feb 19, 2018

marco-c commented Feb 19, 2018

Shashi456 commented Feb 19, 2018

marco-c commented Feb 19, 2018

Shashi456 commented Feb 19, 2018

marco-c commented Feb 19, 2018

Choose a reason for hiding this comment

Shashi456 commented Feb 19, 2018

marco-c commented Feb 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marco-c Feb 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shashi456 commented Feb 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marco-c commented Feb 19, 2018

Shashi456 commented Feb 19, 2018 • edited

marco-c commented Feb 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marco-c left a comment

Choose a reason for hiding this comment

propr bot commented Feb 19, 2018

Shashi456 commented Feb 17, 2018 •

edited by marco-c

poush Feb 17, 2018 •

edited

marco-c Feb 18, 2018 •

edited

marco-c Feb 19, 2018 •

edited

Shashi456 commented Feb 19, 2018 •

edited

Shashi456 commented Feb 19, 2018 •

edited