Add unit tests to the "commentAnalysis.py" #17

pramodrj07 · 2022-01-04T01:03:51Z

Adds unit tests using the 'unittest' framework.
Adds documentation to run all the unit tests and individual modules

Fixes: #8

ml-conversational-analytic-tool/test_commentAnalysis.py

ericwb · 2022-01-04T22:38:36Z

ml-conversational-analytic-tool/test_commentAnalysis.py

+        """ positive test """
+        self.assertEqual(result, 0)
+        """ negative test """
+        self.assertNotEqual(result, 1)


Suggested change

self.assertNotEqual(result, 1)

self.assertNotEqual(result, 1)

Suggested change

self.assertNotEqual(result, 1)

self.assertNotEqual(result, 1)

Add newlines at the end of files.

ericwb · 2022-01-04T23:19:57Z

ml-conversational-analytic-tool/test_commentAnalysis.py

+    def test_CommentAnalyzer_init(self):
+        analyzer = CommentAnalyzer(['test'])
+        """ positive test """
+        self.assertEqual(analyzer.word_count, {'test': 0})


The syntax for assertEqual is (expected, actual). It's common to mistakenly have these swapped, but it matters when test failures are reported.

ericwb · 2022-01-04T23:22:27Z

ml-conversational-analytic-tool/test_commentAnalysis.py

+        """ negative test """
+        self.assertNotEqual(analyzer.word_count, {'test': 1})
+
+    def test_analyze_comments(self):


The naming scheme for tests is "test_" + name of the function

ml-conversational-analytic-tool/test_commentAnalysis.py

ericwb · 2022-01-04T23:28:44Z

ml-conversational-analytic-tool/test_commentAnalysis.py

+    def test_preProcess(self):
+        analyzer = CommentAnalyzer(['test'])
+        result = analyzer.preProcess("This is a test comment")
+        """ positive test """


Stating positive or negative isn't necessary. It's also not that informative. Instead I suggest each function have a docstring explaining the test.

makes sense!! will change this accordingly

ericwb · 2022-01-04T23:29:31Z

ml-conversational-analytic-tool/test_commentAnalysis.py

@@ -0,0 +1,53 @@
+import unittest
+from commentAnalysis import CommentAnalyzer


PEP8 standard: add blank line between standard imports and local.

ericwb · 2022-01-10T21:26:58Z

ml-conversational-analytic-tool/tests/test_commentAnalysis.py

@@ -0,0 +1,64 @@
+import unittest
+
+from ..commentAnalysis import CommentAnalyzer


Suggested change

from ..commentAnalysis import CommentAnalyzer

import commentAnalysis

Not sure why those ".." are included. But it is much preferred to import modules, not classes.

ericwb · 2022-01-10T21:28:34Z

ml-conversational-analytic-tool/__init__.py

@@ -0,0 +1 @@
+import os, sys; sys.path.append(os.path.dirname(os.path.realpath(__file__)))


I'm not clear why the system path is being updated here. But also, the last line of a file should end with a newline. And use one import per module to simplify the import section.

this is required to make the modules(commentAnalysis.py in this case) visible to the testing modules. There are a couple of ways to do it and this seems to work.

ericwb · 2022-01-10T21:30:06Z

README.md

@@ -148,6 +149,10 @@ Both `BaseCNN` and `BaseLSTM` also have prediction explanation mechanisms that c
 If you have ideas on how to improve the framework to assess text conversation for constructive and inclusive
 communication, we welcome your contributions!

+## Running tests
+```python
+python3 -m unittest ml-conversational-analytic-tool/tests/<test_file>.py


So this has to be manually run for each unit test file? How about an automated test suite runner?

we can use python3 -m unittest discover -s tests to run all the tests in one go. Did you mean to say we need to use something like tox when you refer "automated test runner"?

The example given in your README change states how to test file-by-file, not as a collection of tests. Tox isn't a requirement, but it can be effective.

I think it would be helpful to include both

a command for executing all tests

a command for executing one specific file

and you shouldn't need to specify python3 here as that's already defined in the virtualenv

Sounds reasonable. Will do update the README.md for both!

annajung · 2022-01-11T19:46:33Z

README.md

@@ -148,6 +149,10 @@ Both `BaseCNN` and `BaseLSTM` also have prediction explanation mechanisms that c
 If you have ideas on how to improve the framework to assess text conversation for constructive and inclusive
 communication, we welcome your contributions!

+## Running tests
+```python
+python3 -m unittest ml-conversational-analytic-tool/tests/<test_file>.py


I think it would be helpful to include both

a command for executing all tests

a command for executing one specific file

and you shouldn't need to specify python3 here as that's already defined in the virtualenv

ml-conversational-analytic-tool/tests/test_commentAnalysis.py

README.md

ericwb · 2022-01-13T23:25:50Z

README.md

@@ -148,6 +149,16 @@ Both `BaseCNN` and `BaseLSTM` also have prediction explanation mechanisms that c
 If you have ideas on how to improve the framework to assess text conversation for constructive and inclusive
 communication, we welcome your contributions!

+## Running tests


Isn't a virtualenv required here? Could you include that in the directions?

ericwb · 2022-01-13T23:26:49Z

ml-conversational-analytic-tool/__init__.py

@@ -0,0 +1,4 @@
+import os


I still think something is incorrect in how the test suite is configured or run if an init.py is required in the root of the project. I've never seen the need for this before.

Fixed a PATH in my local env variable which fixes this. We do not need init.py now for the imports!

ericwb · 2022-01-13T23:35:35Z

Please also expand on the PR message to be more descriptive. Also include a reference to #8, the issue you're fixing. Thanks

ericwb · 2022-01-18T15:56:25Z

README.md

+## Running tests
+### Run tests with a virtual environment
+```python
+python3 -m venv virtualenv-ml-conversational


Inconsistent use of "python" vs "python3" in the README here

python3 is required when creating a virtual env and is no longer needed when running commands in a virtualenv.

There are already steps in the "Installation" on how to create a virtual env and activate it.

Can we move the "running tests" section inside the "Build and Run" section to leverage the installation step so there are no duplicates?

I'd argue that python3 is unnecessary as python2 is end-of-life. So depending on the users environment, python could map to python3 anyway. It depends on how many python versions are installed on your system. MacOS comes with a Python 2.7 as a system default, but other operating systems default with a Py3.x version. On debian for example, you can define with version python executable points to via update-alternatives.

Since we specify in the prerequisite section that the python version needed is 3.6+, I agree that python3 is not necessary. I think with the minimum version written, we can safely assume that python will be python3.

@pramodrj07 could you also help clean up python3 from line 56?

I was thinking of changing line 56 is a separate PR. But I can do it in the next change.

The command here does not match the title. This only creates a virtual environment and does not run any tests. Also, I believe you would need an extra step to install requirements.txt as well to be able to run the tests.

I suggested adding "Running test" as part of the "Environment Setup" before. This could help remove the duplicate steps in the test section. WDYT?

Yes. I did move it to the 'Environment setup' section

ericwb · 2022-01-18T15:56:55Z

ml-conversational-analytic-tool/tests/test_commentAnalysis.py

@@ -0,0 +1,52 @@
+import unittest
+from unittest.mock import MagicMock


The best practice is to import only modules, not classes (ie MagicMock).

annajung

This mostly looks good to me, just commented on the README clean up

annajung · 2022-01-18T21:41:26Z

README.md

+## Running tests
+### Run tests with a virtual environment
+```python
+python3 -m venv virtualenv-ml-conversational


python3 is required when creating a virtual env and is no longer needed when running commands in a virtualenv.

There are already steps in the "Installation" on how to create a virtual env and activate it.

Can we move the "running tests" section inside the "Build and Run" section to leverage the installation step so there are no duplicates?

1.'unittest' framework of python is used to develop unit tests 2. adds the __init__.py to have standard packaging template

pramodrj07 · 2022-01-19T23:47:37Z

just commented on the README clea

Done!! Moved the testing under 'build and run' section

README.md

update the README.md with the commands to run unit tests

annajung

All comments addressed. Thank you!! LGTM

ericwb

LGTM

pramodrj07 requested review from annajung, difince, enyinna1234 and tzstoyanov as code owners January 4, 2022 01:03

vmwclabot added the cla-not-required label Jan 4, 2022

pramodrj07 force-pushed the unit-tests branch from b07875d to e6238cd Compare January 4, 2022 01:09

pramodrj07 self-assigned this Jan 4, 2022

annajung reviewed Jan 4, 2022

View reviewed changes

ericwb suggested changes Jan 4, 2022

View reviewed changes

pramodrj07 force-pushed the unit-tests branch from e6238cd to 5ff3311 Compare January 10, 2022 19:09

ericwb reviewed Jan 10, 2022

View reviewed changes

annajung suggested changes Jan 11, 2022

View reviewed changes

pramodrj07 force-pushed the unit-tests branch from 5ff3311 to b874070 Compare January 13, 2022 22:43

annajung reviewed Jan 13, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

ericwb suggested changes Jan 13, 2022

View reviewed changes

pramodrj07 force-pushed the unit-tests branch from b874070 to 502e2a3 Compare January 14, 2022 00:57

pramodrj07 changed the title ~~WIP: Add unit tests to the "commentAnalysis.py"~~ Add unit tests to the "commentAnalysis.py" Jan 18, 2022

ericwb reviewed Jan 18, 2022

View reviewed changes

pramodrj07 force-pushed the unit-tests branch from 502e2a3 to 0b9cb9b Compare January 18, 2022 20:12

annajung reviewed Jan 18, 2022

View reviewed changes

Add unit tests to the "commentAnalysis.py"

42b4243

1.'unittest' framework of python is used to develop unit tests 2. adds the __init__.py to have standard packaging template

pramodrj07 force-pushed the unit-tests branch 2 times, most recently from 5e55812 to ed5698f Compare January 19, 2022 23:46

annajung reviewed Jan 20, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

pramodrj07 force-pushed the unit-tests branch from ed5698f to 2353db0 Compare January 20, 2022 19:00

pramodrj07 changed the title ~~Add unit tests to the "commentAnalysis.py"~~ Add unit tests to the "commentAnalysis.py" https://github.com/vmware-labs/ml-conversational-analytic-tool/issues/8 Jan 20, 2022

pramodrj07 changed the title ~~Add unit tests to the "commentAnalysis.py" https://github.com/vmware-labs/ml-conversational-analytic-tool/issues/8~~ Add unit tests to the "commentAnalysis.py" Jan 20, 2022

Add documentation to run unit tests

3d4c96a

update the README.md with the commands to run unit tests

pramodrj07 force-pushed the unit-tests branch from 2353db0 to 3d4c96a Compare January 20, 2022 19:36

annajung approved these changes Jan 20, 2022

View reviewed changes

pramodrj07 requested a review from ericwb January 20, 2022 21:02

ericwb approved these changes Jan 20, 2022

View reviewed changes

ericwb merged commit 5262fea into vmware-archive:main Jan 20, 2022

	self.assertNotEqual(result, 1)
	self.assertNotEqual(result, 1)

		@@ -0,0 +1,53 @@
		import unittest
		from commentAnalysis import CommentAnalyzer

		@@ -0,0 +1,64 @@
		import unittest

		from ..commentAnalysis import CommentAnalyzer

	from ..commentAnalysis import CommentAnalyzer
	import commentAnalysis

		@@ -0,0 +1 @@
		import os, sys; sys.path.append(os.path.dirname(os.path.realpath(__file__)))

		@@ -0,0 +1,52 @@
		import unittest
		from unittest.mock import MagicMock

Add unit tests to the "commentAnalysis.py" #17

Add unit tests to the "commentAnalysis.py" #17

Conversation

pramodrj07 commented Jan 4, 2022 • edited by ericwb Loading

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

ericwb Jan 4, 2022 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericwb commented Jan 13, 2022

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

Choose a reason for hiding this comment

annajung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pramodrj07 commented Jan 19, 2022

annajung left a comment

Choose a reason for hiding this comment

ericwb left a comment

Choose a reason for hiding this comment

pramodrj07 commented Jan 4, 2022 •

edited by ericwb

Loading

ericwb Jan 4, 2022 •

edited

Loading