Unit tests #9

aburrell · 2020-10-02T13:28:37Z

Updated unit tests, partially addressing issue #8

Test files are currently missing, but a skipIf command is in place
You can run the tests from the pyDARNio directory using: python3 -m unittest discover

The Borealis tests are currently failing. It looks like the testing data in the borealis_*_data_sets.py files are not up to snuff. @mardet987 I would appreciate you taking a look at this.

Added a missing definition of `long_description`, allowing setup.py to run.

Added autosaves to the list of files for git to ignore.

…develop

Updated the unit tests in TestSDarnRead by combining similar unit tests with subtest, ensuring file structure works for all operating systems, and only skipping test class if test file directory is missing.

Updated DarnUtilities unit tests by: - using setup/teardown for common variables, - replacing pytest skip with unittest skip, - used subTest to combine common tests, - fixed implementation of exception tests, and - removed name call at end of file.

Fixed incorrect case in `pyDARNio` imports.

Fixed incorrect case in pyDARNio import.

Fixed incorrect case in pyDARNio import and improved PEP8 spacing.

Fixed bugs in testable unit tests, removed unnecessary skip, and fixed import statements.

Replaced deprecated `tostring` with `tobytes`.

Updated write_rawacf to include the correct class structures.

Updated test_superdarn by: - fixing subTest implementation, - removing unnecessary skips, - removing duplicate tests in SDarnWrite class, and - using setUp and tearDown in SDarnWrite class.

Created a routine to retrieve the desired test filenames.

Updated error message to fix formatting and add missing whitespace.

Fixed incorrect import of pyDARNio.

Created a common test class for pyDARNio file reading, reducing duplicate code in test_superdarn and test_dmap.

Moved `tfile_utils` to `file_utils`, since it's a better name. Also updated `get_test_files` to retrieve the Borealis test files.

Created a common test writing class for SDarn and Dmap writing class unit tests.

Fixed bugs in unit test caused by: - typos and - incorrect attribute definition.

Changed pydarn import to pyDARNio import.

Updated test_conversions to: - import pyDARNio correctly, - include a tearDown function, - use local class attributes in helper functions, - use subTest, and - remove the unnecessary __name__ statement at the end of the file.

Added a TestReadBorealis class that reduces the duplication found in the previous Borealis reading unit testing classes.

Used more robust method of obtaining a list from keys.

Created a BorealisWrite generic class, but is not currently working for unknown reasons.

Consolidated Borealis test classes: - placed in their own utility file, - removed duplicate tests using subTest, and - ensure all version/structure combos are run using test classes.

unittest discover requires the presence of `__init__.py` files to find the test scripts.

mts299

@aburrell I am noticing a lot of changes with no comments, and ones that don't match the previous code. If these changes are on preference then actual reason. I would prefer them to go back to normal.

Also, pyDARNio will break any install where PyPI is uploaded with pydarnio. PLEASE if you are going to make these decisions then make an issue to discuss.

pydarnio/borealis/base_format.py

pydarnio/borealis/borealis_site.py

pydarnio/dmap/dmap.py

aburrell · 2020-10-09T14:38:54Z

Right. The changes are to remove the duplication and make the tests easier to maintain. It's a big overhaul, so it's going to look different. However, it addresses most of the "TODO" in the unit tests. I would be happy to go over the new unit testing structure with the pyDARNio crew if people are interested.

WRT comments, the tests are the same tests as before, with some additional coverage and smart skipping. All of the tests have descriptive docstrings. I haven't put descriptive docstrings in the test classes, but can add those if the developers would like them.

pydarnio/dmap/dmap.py

tests/unit/file_utils.py

tests/unit/test_dmap.py

mts299 · 2020-10-09T14:47:04Z

tests/unit/test_dmap.py

-        with self.assertRaises(pydarnio.dmap_exceptions.DmapCharError):
-            dmap_write.dmap_scalar_to_bytes(scalar)
-
-    def test_String_array(self):


Why are you removing these tests?

Current setup.py correctly imports pyDARNio as pydarnio.

Fixed typo in method name. Co-authored-by: Marina Schmidt <marina.t.schmidt@gmail.com>

mts299 · 2020-10-09T15:18:34Z

Sorry, @aburrell there is a lot of changes here and not just unit tests which don't make sense to me or why you changed them.

You apparently fixed bugs, wrote a new test or file getter, and edited a lot on the unit tests and this is too much to review in one PR.

The scope is a bit wide and I probably asking "why?" a lot. Somethings like import pyDARNio vs. import pydarnio confuses me and its apparently a bug for only you. But I know nothing of the details.

I would recommend making multiple branches that reflect each portion of changes and updates. Also, if you pick things based on preferences then make a PR on that, some people like their code to reflect their style so changing it might be a bit triggering like myself.

Also shouldn't the file getting test thing to be part of when we make a test data set?

Otherwise, not sure I can review this anytime soon.

mts299 · 2020-10-09T15:24:37Z

Thank you for all this code and for looking over everything! I would just like to have more focus on what is going on in many of the changes. Hopefully, separate branches are not too much of a pain.

aburrell · 2020-10-09T15:48:20Z

You may or may not be surprised to know that this already a sub-branch of this project. It's a big restructure, that I spent a fair bit of time scoping to ensure that all the original work was kept while making it easier to maintain and run in the future.

This PR could be broken into DMap/SuperDARN changes and Borealis changes, but I don't know how to do that (my GitFoo is fairly basic).

What I could do to help reviewers and future development is create a document describing the new structure, how it applies to the old structure, and what's useful about the new structure. I could see from the tone of your comments that "[...] changing it might be a bit triggering like myself." was in play, but I did not change anything without thinking through best practices for unit tests and considering multiple alternatives. None of the changes are based on my stylistic preferences. Hopefully the document will help the current and future developers with unit and integration tests.

Just a note, that I am currently on 0% SuperDARN time, so it will likely be a while before I get the testing structure file posted to the wiki or another suitable location. When I do I'll link it here.

mts299 · 2020-10-09T16:00:06Z

sub-branch? Should this be a draft PR then or merging not to develop?

Well make branches and copy the changes over that fit the scope. Possible branches:

import pyDARNio
preferences or updates to DMap (explanations)
preferences or updates to SuperDARN
preferences or updates to Borealis
updates to unit test structure
preferences or changes to current unit tests
file reading for unit tests

If you did find any other BUGS then make those a PR and issue.
The new structure of pydarnio or unit tests? Why are we changing so much?

Documentation would help but separate PRs and issues would also help.
You haven't tested the new structure?

pydarnio/dmap/dmap.py

pydarnio/exceptions/dmap_exceptions.py

mts299 · 2020-10-09T16:05:48Z

tests/unit/borealis_utils.py

@@ -0,0 +1,471 @@
+# Copyright (C) 2020 NRL


What is this test file for?

mts299 · 2020-10-09T16:07:12Z

tests/unit/borealis_utils.py

+    Testing class for reading Borealis data
+    """
+
+    def setUp(self):


mts299 · 2020-10-09T16:08:25Z

tests/unit/borealis_utils.py

+import borealis_antennas_iq_data_sets as borealis_antennas_iq
+
+
+def get_borealis_type(file_type, file_struct, version):


Suggested change

def get_borealis_type(file_type, file_struct, version):

def get_borealis_type(file_type: str, file_struct: str, version: int):

mts299 · 2020-10-09T16:08:54Z

tests/unit/borealis_utils.py

+        del self.test_file, self.test_dir, self.data, self.rec, self.arr
+        del self.read_func, self.file_types, self.file_struct, self.version
+
+    def load_file_record(self, file_type=''):


Suggested change

def load_file_record(self, file_type=''):

def load_file_record(self, file_type: str = ''):

mts299 · 2020-10-09T16:09:46Z

tests/unit/borealis_utils.py

+        self.temp_file = "{:s}_{:s}_test.{:s}.hdf5".format(
+            self.data_type, self.file_struct, self.data_type)
+
+    def load_temp_file(self, file_type=''):


Suggested change

def load_temp_file(self, file_type=''):

def load_temp_file(self, file_type=''):

Is there a change here? It looks identical to me.

tests/unit/file_utils.py

mts299 · 2020-10-09T16:11:09Z

tests/unit/file_utils.py

+import map_data_sets
+import rawacf_data_sets
+
+


Can you add typing in all your function calls to be consistent?

Yes, I can do that.

Fixed remaining imports, reverting to `pydarnio`.

aburrell · 2020-10-09T16:38:54Z

sub-branch? Should this be a draft PR then or merging not to develop?

No, the upcoming changes will be a draft once they're ready

Well make branches and copy the changes over that fit the scope. Possible branches:
import pyDARNio
Already taken care of in ef9f62c and 39cc7fd

preferences or updates to DMap (explanations)
preferences or updates to SuperDARN
preferences or updates to Borealis

They are a very small number of changes to make the code flake8 compliant and robust across python 3 versions. Doesn't seem worth the effort.

updates to unit test structure
preferences or changes to current unit tests
file reading for unit tests

These are all linked together

If you did find any other BUGS then make those a PR and issue.
Fair point; it's the one pydarn import.

The new structure of pydarnio or unit tests? Why are we changing so much?
I've addressed this multiple times in comments and told you I will supply more detail in documentation.

Documentation would help but separate PRs and issues would also help.
I'll do what I can given the time and resources available to me.

You haven't tested the new structure?
Of course I've tested the new structure. As stated in my top comment when creating this PR everything passes locally but Borealis, which was failing beforehand anyhow. As I said in the previous comment, separating DMap/SuperDARN and Borealis would probably be helpful here. But, as I said in the previous comment, I don't know how to do that.

Given your review thus far what I need from you is help learning how to separate this PR into separate PRs.

I will also address the important style issues you identified (e.g. missing docstrings and type hinting), but will not be working on this PR or commenting on it again for at least a week.

mardet987 · 2020-10-16T16:12:17Z

Hey @aburrell I tried testing using
python3 -m unittest discover
Like you suggested but had a couple of issues.

If I run from the pydarnio package's root dir as suggested, I get this error, I guess it is not finding the module to import, similar with the file_utils for test_dmap and test_superdarn:

======================================================================
ERROR: tests.unit.test_borealis (unittest.loader._FailedTest)

ImportError: Failed to import test module: tests.unit.test_borealis
Traceback (most recent call last):
File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/unittest/loader.py", line 434, in _find_test_path
module = self._get_module_from_name(name)
File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/unittest/loader.py", line 375, in _get_module_from_name
import(name)
File "/Users/Marci/code/pydarnio/tests/unit/test_borealis.py", line 23, in
import borealis_utils
ModuleNotFoundError: No module named 'borealis_utils'

I then tried the same thing from just within the test/unit subdirectory so the above wasn't an issue. I saw some mention of this issue in the discussion above.

======================================================================
ERROR: test_dmap (unittest.loader._FailedTest)

ImportError: Failed to import test module: test_dmap
Traceback (most recent call last):
File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/unittest/loader.py", line 434, in _find_test_path
module = self._get_module_from_name(name)
File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/unittest/loader.py", line 375, in _get_module_from_name
import(name)
File "/Users/Marci/code/pydarnio/tests/unit/test_dmap.py", line 11, in
import file_utils
File "/Users/Marci/code/pydarnio/tests/unit/file_utils.py", line 14, in
import dmap_data_sets
File "/Users/Marci/code/pydarnio/tests/unit/dmap_data_sets.py", line 6, in
from pyDARNio import DmapArray, DmapScalar
ModuleNotFoundError: No module named 'pyDARNio'

The capitalization is messing up the import, I tried 'import pyDARNio' in the interpreter and got the error as well. Is there a plan to fix this? I can work around these to get to the Borealis testing issues for now.

more revert to pydarnio in data set files

mardet987 · 2020-10-16T16:48:29Z

I just noticed that you reverted some imports in other files so I reverted them where I was having more issues.

Data sets had some additional fields

Array tests work now.

mardet987 · 2020-10-16T21:24:07Z

Update: still working on this and the read unittests

Errors now have better descriptions for ease of use.

tests/unit/file_utils.py

Fixed glob usage, which does not require unix commands to execute. Co-authored-by: mardet987 <mardet987@users.noreply.github.com>

Bug fix of test_convert_to_dmap clarified file extension as 'hdf5'

mardet987 · 2020-10-30T23:23:32Z

Ok I completed the read tests now without issue!

@mts299 I remember discussing putting test files somewhere, where could I put the testdir that I used for this?

Also, I was wondering if it is possible for us to discuss the pydarnio_logger.error() in the exception initiation. It prints the error to screen which can be misleading if the error then doesn't get raised (for example, in a try/except which I use in the BorealisRead and BorealisWrite classes to determine the version and file structure). The one that I've found has been an issue is line 344 in borealis_exceptions.py which prints the Structure error even though it is often caught and the correct structure subsequently found.

mts299

I think this might be my last review before approving it. Some things I am still what is going on.

mts299 · 2020-11-02T17:35:02Z

tests/unit/file_utils.py

+    """ Testing class for reading classes
+    """
+
+    def setUp(self):


Can you add docstring on setUP and tearDown on what they are doing?

mts299 · 2020-11-02T17:36:27Z

tests/unit/file_utils.py

+class TestWrite(unittest.TestCase):
+    """ Testing class for writing classes
+    """
+    def setUp(self):


Docstrinngs here as well and for other missing ones on test classes

mts299 · 2020-11-02T17:37:48Z

tests/unit/test_conversions.py

@@ -7,6 +7,7 @@
 from collections import OrderedDict

 import pydarnio


Suggested change

import pydarnio

mts299 · 2020-11-02T17:41:41Z

tests/unit/test_dmap.py

@@ -1,298 +1,99 @@
 # Copyright (C) 2019 SuperDARN Canada, University of Saskatchewan
-# Author: Marina Schmidt
+# Author: Marina Schmidt, Angeline Burrell


Suggested change

# Author: Marina Schmidt, Angeline Burrell

# Author: Marina Schmidt

# Modification:

# 2020-11 Angeline Burrell converted pydarnio tests to pytests and updated that code to have teardowns and setups.

Actually, you would need to change the overall functionality and meaning to add your name as an Author.

mts299 · 2020-11-02T17:42:21Z

tests/unit/test_dmap.py

-        with self.assertRaises(pydarnio.dmap_exceptions.NegativeByteError):
-            dmap.read_records()
-
-    def test_dmap_read_stream(self):


Why are you removing all these tests?

mts299 · 2020-11-02T17:46:18Z

@mardet987 @aburrell where would you like the test files?

Ther pydarn.logger.error() should not activate unless it's set on? Did you do that or wise this may be a bug

mardet987 · 2020-11-03T19:54:26Z

pydarnio/exceptions/borealis_exceptions.py

-                " supported). Please check if you are interested"\
-                " in records or arrays."\
-                "{error_str}".format(error_str=error_str)
+        self.message = error_str
        pyDARNio_logger.error(self.message)


@mts299 this is the line I'm talking about.

I didn't change any pydarnio_logger settings, but this wrote to stdout even though the exception was later caught.

Did we discuss FRDR or CEDAR for a directory of test files?

huh... might need to look into that?

Yes looks like you found it on the DAWG repo?

is this call supposed to write to stdout?

Yes I found the DAWG issue, I can upload files wherever but I don't have experience with FRDR. If that's the choice, please point me to the location where I should upload!

aburrell added 26 commits July 17, 2020 08:47

BUG: long descritption

74b639c

Added a missing definition of `long_description`, allowing setup.py to run.

ENH: ignore autosaves

e6c02e7

Added autosaves to the list of files for git to ignore.

Merge branch 'develop' of https://github.com/SuperDARN/pyDARNio into …

a301b39

…develop

TST: updated TestSDarnRead

ae7e6b8

Updated the unit tests in TestSDarnRead by combining similar unit tests with subtest, ensuring file structure works for all operating systems, and only skipping test class if test file directory is missing.

BUG: import pyDARNio

676f1d2

Fixed incorrect case in `pyDARNio` imports.

BUG: import pyDARNio

e6bf7d0

Fixed incorrect case in pyDARNio import.

BUG: import pyDARNio and whitespace

143d0f7

Fixed incorrect case in pyDARNio import and improved PEP8 spacing.

BUG: test_superdarn pyDARNio imports and unit test bugs

770b979

Fixed bugs in testable unit tests, removed unnecessary skip, and fixed import statements.

MAINT: remove use of deprecated function

864c311

Replaced deprecated `tostring` with `tobytes`.

BUG: updated write_rawacf

0001af3

Updated write_rawacf to include the correct class structures.

TST: updated test_superdarn

2b8b7f3

Updated test_superdarn by: - fixing subTest implementation, - removing unnecessary skips, - removing duplicate tests in SDarnWrite class, and - using setUp and tearDown in SDarnWrite class.

TST: created general file testing routine

a26d732

Created a routine to retrieve the desired test filenames.

STY: updated error message

987a56d

Updated error message to fix formatting and add missing whitespace.

BUG: incorrect import

6822d35

Fixed incorrect import of pyDARNio.

TST: created test class for pyDARNio file reading

35de9f5

Created a common test class for pyDARNio file reading, reducing duplicate code in test_superdarn and test_dmap.

TST: file_utils naming

ed2c55a

Moved `tfile_utils` to `file_utils`, since it's a better name. Also updated `get_test_files` to retrieve the Borealis test files.

TST: created common TestWrite class

26cc3d7

Created a common test writing class for SDarn and Dmap writing class unit tests.

BUG: fixed unit test bugs

08937cc

Fixed bugs in unit test caused by: - typos and - incorrect attribute definition.

BUG: pyDARNio import

aa6b16f

Changed pydarn import to pyDARNio import.

TST: updated unit test_conversions

ccf7d1a

Updated test_conversions to: - import pyDARNio correctly, - include a tearDown function, - use local class attributes in helper functions, - use subTest, and - remove the unnecessary __name__ statement at the end of the file.

TST: added TestReadBorealis class

3cf1ffd

Added a TestReadBorealis class that reduces the duplication found in the previous Borealis reading unit testing classes.

STY: list from keys

d31a18f

Used more robust method of obtaining a list from keys.

TST: BorealisWrite class

7478307

Created a BorealisWrite generic class, but is not currently working for unknown reasons.

TST: borealis general classes

bbd5f14

Consolidated Borealis test classes: - placed in their own utility file, - removed duplicate tests using subTest, and - ensure all version/structure combos are run using test classes.

TST: added missing init files

af3b9de

unittest discover requires the presence of `__init__.py` files to find the test scripts.

aburrell requested review from mts299 and mardet987 October 5, 2020 13:00

aburrell mentioned this pull request Oct 6, 2020

pydarnio class in pyDARN SuperDARN/pydarn#78

Merged

mts299 requested changes Oct 9, 2020

View reviewed changes

pydarnio/borealis/base_format.py Outdated Show resolved Hide resolved

pydarnio/borealis/borealis_site.py Show resolved Hide resolved

pydarnio/dmap/dmap.py Outdated Show resolved Hide resolved

mts299 reviewed Oct 9, 2020

View reviewed changes

aburrell and others added 2 commits October 9, 2020 10:50

BUG: pyDARNio import

ef9f62c

Current setup.py correctly imports pyDARNio as pydarnio.

MAINT: typo in definition name

ac8a5a4

Fixed typo in method name. Co-authored-by: Marina Schmidt <marina.t.schmidt@gmail.com>

mts299 reviewed Oct 9, 2020

View reviewed changes

BUG: import updates

39cc7fd

Fixed remaining imports, reverting to `pydarnio`.

BUG: import updates

e553eea

more revert to pydarnio in data set files

mardet987 added 2 commits October 16, 2020 15:19

BUG: borealis_antennas_iq_data_sets

5ccd601

Data sets had some additional fields

Improved tests for varying file_struct

3e5a720

Array tests work now.

BorealisStructureError improvements

7e129c4

Errors now have better descriptions for ease of use.

mardet987 reviewed Oct 19, 2020

View reviewed changes

tests/unit/file_utils.py Outdated Show resolved Hide resolved

aburrell and others added 2 commits October 19, 2020 22:49

BUG: glob usage

ed9d99b

Fixed glob usage, which does not require unix commands to execute. Co-authored-by: mardet987 <mardet987@users.noreply.github.com>

Improved version number in test

d1191dd

Bug fix of test_convert_to_dmap clarified file extension as 'hdf5'

mts299 requested changes Nov 2, 2020

View reviewed changes

mardet987 reviewed Nov 3, 2020

View reviewed changes

aburrell mentioned this pull request Jun 28, 2021

Integration tests #24

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unit tests #9

Unit tests #9

aburrell commented Oct 2, 2020

mts299 left a comment

aburrell commented Oct 9, 2020

mts299 Oct 9, 2020

mts299 commented Oct 9, 2020

mts299 commented Oct 9, 2020

aburrell commented Oct 9, 2020

mts299 commented Oct 9, 2020

mts299 Oct 9, 2020

mts299 Oct 9, 2020

mts299 Oct 9, 2020

mts299 Oct 9, 2020

mts299 Oct 9, 2020

aburrell Oct 9, 2020

mts299 Oct 9, 2020

aburrell Oct 9, 2020

aburrell commented Oct 9, 2020

mardet987 commented Oct 16, 2020 •

edited

======================================================================
ERROR: tests.unit.test_borealis (unittest.loader._FailedTest)

======================================================================
ERROR: test_dmap (unittest.loader._FailedTest)

mardet987 commented Oct 16, 2020

mardet987 commented Oct 16, 2020

mardet987 commented Oct 30, 2020

mts299 left a comment

mts299 Nov 2, 2020

mts299 Nov 2, 2020

mts299 Nov 2, 2020

mts299 Nov 2, 2020

mts299 Nov 2, 2020

mts299 commented Nov 2, 2020

mardet987 Nov 3, 2020

mts299 Nov 3, 2020

mardet987 Nov 3, 2020

		import borealis_antennas_iq_data_sets as borealis_antennas_iq


		def get_borealis_type(file_type, file_struct, version):

	def get_borealis_type(file_type, file_struct, version):
	def get_borealis_type(file_type: str, file_struct: str, version: int):

	def load_file_record(self, file_type=''):
	def load_file_record(self, file_type: str = ''):

	def load_temp_file(self, file_type=''):
	def load_temp_file(self, file_type=''):

		@@ -7,6 +7,7 @@
		from collections import OrderedDict

		import pydarnio

Unit tests #9

Are you sure you want to change the base?

Unit tests #9

Conversation

aburrell commented Oct 2, 2020

mts299 left a comment

Choose a reason for hiding this comment

aburrell commented Oct 9, 2020

Choose a reason for hiding this comment

mts299 commented Oct 9, 2020

mts299 commented Oct 9, 2020

aburrell commented Oct 9, 2020

mts299 commented Oct 9, 2020

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

aburrell commented Oct 9, 2020

mardet987 commented Oct 16, 2020 • edited

====================================================================== ERROR: tests.unit.test_borealis (unittest.loader._FailedTest)

====================================================================== ERROR: test_dmap (unittest.loader._FailedTest)

mardet987 commented Oct 16, 2020

mardet987 commented Oct 16, 2020

mardet987 commented Oct 30, 2020

mts299 left a 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

mts299 commented Nov 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mardet987 commented Oct 16, 2020 •

edited

======================================================================
ERROR: tests.unit.test_borealis (unittest.loader._FailedTest)

======================================================================
ERROR: test_dmap (unittest.loader._FailedTest)