Cdf attribute tests #637

anamanica · 2024-06-17T20:07:20Z

Change Summary

Overview

Creating CDF attribute code tests for the two files:
1.) cdf_attribute_manager.py
2.) imap_cdf_manager.py

New Dependencies

None

New Files

For CDF Attribute Manager:

imap_default_global_test_cdf_attribues.yaml
- Contains test .yaml for global variables
- General info (tests load_global_attributes)
- Also used in testing variable attributes
imap_test_global.yaml
- Contains test .yaml for global cdf attributes
- INSTRUMENT SPECIFIC (tests get_global_attributes)
imap_test_variable.yaml
- Contains test .yaml for variable cdf attributes
- INSTRUMENT SPECIFIC (tests get_variable_attributes)

For IMAP CDF Manager

test_imap_cdf_manager.py
- Contains unit tests for imap_cdf_manager.py
imap_instrument_global_cdf_attrs.yaml
- Tests add_instrument_global_attrs
- Same information as imap_test_global.yaml, but needed a specific file naming convention.
imap_instrument2_global_cdf_attrs.yaml
- Tests add_instrument_global_attrs
- Same information as imap_swe_global.yaml, but needed a specific file naming convention.
imap_instrument_level_variable_attrs.yaml
- Tests add_instrument_variable_attrs
- Same information as imap_test_variable.yaml, but again, needed a specific file naming convention
imap_instrument2_level2_variable_attrs.yaml
- Tests add_instrument_variable_attrs
- Same information as imap_ultra_l1b_variable.yaml, but again, needed a specific file naming convention

Deleted Files

None

Updated Files

test_cdf_attribute_manager.py
- Creating and improving unit tests
imap_cdf_manager.py
- Added an optional parameter to __init__ so that I could reach my test files while also preserving the API.

Testing

The purpose of this ticket is to specifically create unit tests, and thus all the work this ticket is unit testing.

closes #592

maxinelasp

Looks like a great first start! Not approving only because it's a draft rn, but it's looking really good!

maxinelasp · 2024-06-18T14:07:43Z

imap_processing/cdf/config/imap_instrument_level_variable_attrs.yaml

@@ -0,0 +1,23 @@
+default_attrs: &default


These YAML files look good, but they should go under tests/

maxinelasp · 2024-06-18T14:10:24Z

imap_processing/cdf/config/imap_test_global.yaml

+    Logical_source_description: IMAP Mission MAGi Normal Rate Instrument Level-1A Data.
+
+
+imap_test_section_5:


You don't need more than 2 imap_test_sections IMO. What is adding additional sections adding from a test perspective? One test is good to validate that you can get into the section. Two is good to validate that you can choose between the sections. But adding additional sections doesn't test anything extra.

maxinelasp · 2024-06-18T14:14:31Z

imap_processing/cdf/tests/test_cdf_attribute_manager.py

+
+    # Default global tests
+    # Check false case
+    assert cdf_manager.global_attribute_schema["DOI"]["required"] is False


It's a very minor thing, but you can also check falseness by saying assert not [statement] or check trueness with assert [statement]

maxinelasp · 2024-06-18T14:17:03Z

imap_processing/cdf/tests/test_cdf_attribute_manager.py

    cdf_manager = CdfAttributeManager(Path(__file__).parent.parent / "config")
+    # Test that default information was loaded in from "imap_default_global_cdf_attrs.yaml"


hm, this might actually be a bug within CdfAttributeManger. I don't think it should load in any information to start. This is irrelevant to your PR, just a mental note for me to go back and fix that

Ah! Okay, I see. This is the code that shows information is loaded in:
# Load Default IMAP Global Attributes self.global_attributes = CdfAttributeManager._load_yaml_data( self.source_dir / DEFAULT_GLOBAL_CDF_ATTRS_FILE ) self.variable_attributes = dict()

That is line 78 in cdf_attribute_manager.py, and DEFAULT_GLOBAL_CDF_ATTRS_FILE is defined as

DEFAULT_GLOBAL_CDF_ATTRS_FILE = "imap_default_global_cdf_attrs.yaml"

in line 12.

Hmm ok in that case, leave this test in, but I probably will want to update that lol

Should not block or really affect you

maxinelasp · 2024-06-18T14:17:12Z

imap_processing/cdf/tests/test_cdf_attribute_manager.py

-    cdf_manager.load_global_attributes("imap_default_global_cdf_attrs.yaml")
+    # Load in different data, test what was carried over
+    cdf_manager.load_global_attributes("imap_default_global_test_cdf_attrs.yaml")
+    assert cdf_manager.global_attributes["Project"] == "STP>Solar-Terrestrial Physics"


imap_processing/cdf/tests/test_cdf_attribute_manager.py

maxinelasp · 2024-06-18T14:20:15Z

imap_processing/cdf/tests/test_imap_cdf_manager.py

+
+# test add_instrument_global_attrs(self, instrument: str):
+
+


Nice, I like this file, but don't add much more than this to it. ImapCdfAttributes is very minimal, and you don't want to duplicate tests. Really, the only test needed is that self.source_dir is set correctly, because right now that's the only thing done in ImapCdfAttributes, with everything else needing to be tested in test_cdf_attribute_manager

imap_processing/cdf/tests/test_cdf_attribute_manager.py

maxinelasp · 2024-06-18T18:06:04Z

imap_processing/cdf/tests/test_cdf_attribute_manager.py

@@ -164,9 +197,6 @@ def test_global_attribute():
        if required_schema is True:
            assert attr_name in test_get_global_attrs.keys()

-    # Testing second elif statement
-    # Should throw error


If this is testing that the code successfully throws an error, you can use pytest.raises. If this test is failing because the code it's testing is incomplete or wrong, but it shouldn't, that's when you use pytest.mark.xfail. Basically, if someone should come back and fix it, xfail is your friend.

maxinelasp

Not done with my review yet - but nice work!

maxinelasp · 2024-06-20T19:32:42Z

imap_processing/cdf/imap_cdf_manager.py

+        if source_dir_input is None:
+            super().__init__(Path(__file__).parent / "config")
+        else:
+            super().__init__(Path(__file__).parent / "config")


You should pass the source_dir_input into the super() init function because there are steps in __init__ that depend on the source_dir and it would be confusing to have it set to different values for the init vs for the rest of processing.

Then you can remove line 21

This has actually been causing me a lot of weird struggles with testing this class. In CdfAttributeManager , the default schema and IMAPS default variables are in the "config" file path and not guaranteed in the source_dir_input file path. I can't get the tests to run correctly without loading in that default info first, and then switching the file path... Thoughts?

In those tests, you can pass in the "config" path (or pass in nothing and use the default) and then override it. This way it will still get the default schemas and global variables from "config" but you can also access your test files in the other file path.

Hahaha, happy to say I just got it. Thank you so much! I see now.

imap_processing/cdf/tests/imap_instrument2_global_cdf_attrs.yaml

imap_processing/cdf/tests/imap_instrument2_level2_variable_attrs.yaml

maxinelasp · 2024-06-20T19:39:22Z

imap_processing/cdf/tests/shared/default_global_cdf_attrs_schema_test.yaml

@@ -0,0 +1,246 @@
+DOI:


Is there any way/is it reasonable to use the real schema? I know this goes back to the source_dir question again, but I think it would be valuable to test against the real schema - because if that ever changes, and the person updating doesn't realize that the file is duplicated in the tests, it could cause issues.

Yes! Sorry, I actually am using the real schema, I should have deleted these files. This is why we have to initially set the CdfAttributeManager.source_dir to end in the "config" folder to get those real schema...

imap_processing/cdf/tests/shared/default_variable_cdf_attrs_schema_test.yaml

imap_processing/cdf/tests/test_cdf_attribute_manager.py

maxinelasp · 2024-06-20T20:27:21Z

imap_processing/cdf/tests/test_cdf_attribute_manager.py

+    # Testing that required schema keys are in get_global_attributes
+    for attr_name in cdf_manager.global_attribute_schema.keys():
+        required_schema = cdf_manager.global_attribute_schema[attr_name]["required"]
+        if required_schema is True:


imap_processing/cdf/tests/test_cdf_attribute_manager.py

maxinelasp · 2024-06-20T20:30:13Z

imap_processing/cdf/tests/test_cdf_attribute_manager.py

+    cdf_manager.add_global_attribute("Project", "Test Project")
+    assert cdf_manager.global_attributes["Project"] == "Test Project"
+    test_get_global_attrs = cdf_manager.get_global_attributes("imap_test_T1_test")
+    assert test_get_global_attrs["Project"] == "Test Project"


Nice test, I like it. Can you also add a test for adding a new attribute that didn't exist in the file? For example, delete one of the required attributes (you can delete it out of cdf_manager.global_attributes) and then assert that get_global_attributes fails for that attribute, then run add_global_attribute and assert that it now exists.

Yeah! You will see the updated test in some of my new pushes, but I did have a few questions about this:

The code in get_global_attributes does not throw an error if a required attribute is missing, it just sets that attribute to None. Shall this be changed in the method definition?

I can't add attributes that are instrument specific because all instrument specific data is accessed through dictionary objects. and not CdfAttributeManager objects (such as cdf_manager.global_attributes["instrument"] or instrument = cdf_manager.get_global_attributes("instrument)). Is that okay?

No, don't change the code, but instead validate that it's None.

You can add instrument specific attributes through add_global_attribute because the global attributes are flat and contain the global attributes for the instruments and for the shared values.

I'm not totally sure I understand your second question, did I answer it?

Ah. I think I got it now, thanks!

imap_processing/cdf/tests/test_cdf_attribute_manager.py

maxinelasp

Finished review, ping me when you're ready for another one! Also, let me know if you want to meet to discuss any of this since there are some larger questions here that might be easier to have a conversation about.

maxinelasp · 2024-06-21T15:37:25Z

imap_processing/cdf/tests/test_cdf_attribute_manager.py

+    cdf_manager.add_global_attribute("Project", "Test Project")
+    assert cdf_manager.global_attributes["Project"] == "Test Project"
+    test_get_global_attrs = cdf_manager.get_global_attributes("imap_test_T1_test")
+    assert test_get_global_attrs["Project"] == "Test Project"


No, don't change the code, but instead validate that it's None.

You can add instrument specific attributes through add_global_attribute because the global attributes are flat and contain the global attributes for the instruments and for the shared values.

I'm not totally sure I understand your second question, did I answer it?

imap_processing/cdf/tests/test_imap_cdf_manager.py

maxinelasp · 2024-06-21T16:20:10Z

imap_processing/cdf/tests/test_imap_cdf_manager.py

+    assert imap_instrument["Data_type"] == "T1_test-one>Test-1 test one"
+    assert imap_instrument["Project"] == "STP>Solar-Terrestrial Physics"
+
+    # Testing reloading data


This is already tested in the test_cdf_attribute_manager code - most of these tests are somewhat duplicating it. It's ok to have some duplicate tests, but generally, you should really try to focus in and make sure you only have one test per thing. If we changed the way we add global attrs, we would have to go and update 2 tests that test the same thing, so it's a little annoying.

This isn't a blocker for me since this test file is pretty minimal - so I will leave it up to your judgement which tests are duplicating and which ones are adding value. Just wanted to point that out as something to think about.

The particular reason I included both of these tests is because they are loaded in from different files. That being said though, I definitely hear what you are saying. I'll rake through my tests, and get rid of the silly ones.

maxinelasp

Nice work! Really really close now, if you address my newest comments then you can go ahead and merge! Congrats for making it through the first review thunderdome 😁

imap_processing/cdf/tests/test_cdf_attribute_manager.py

maxinelasp · 2024-06-21T21:56:39Z

imap_processing/cdf/tests/test_imap_cdf_manager.py

-    imap_cdf_manager = ImapCdfAttributes("tests")
+    # Create an ImapCdfAttributes object, set to correct file path
+    imap_cdf_manager = ImapCdfAttributes()
+    imap_cdf_manager.source_dir = Path(__file__).parent.parent / "tests"


imap_processing/cdf/tests/test_imap_cdf_manager.py

imap_processing/cdf/imap_cdf_manager.py

bourque

Awesome work! I just have a few non-blocking nit-picky suggestions, if you so chose to accept them.

imap_processing/cdf/imap_cdf_manager.py

bourque · 2024-06-20T17:43:43Z

imap_processing/cdf/tests/imap_default_global_test_cdf_attrs.yaml

+Mission_group: Dysfunctional Cats
+PI_name: Ana Manica


imap_processing/cdf/imap_cdf_manager.py

bourque · 2024-06-24T19:20:31Z

imap_processing/cdf/tests/imap_instrument_global_cdf_attrs.yaml

+  This file is nothing more than a test yamal. This description? This is a test description.
+  This Testinstrument will contribute to our understanding of the cdf_attribute_manager.py file.
+  This test file is led by me, Ana Manica. Lasp undergraduate employee.
+  I don't have a website. Sorry.


Beautifully written 😂

imap_processing/cdf/tests/imap_instrument_global_cdf_attrs.yaml

bourque · 2024-06-24T19:27:21Z

imap_processing/cdf/tests/test_cdf_attribute_manager.py

+        "CATDESC",
+        "DEPEND_0",
        "DISPLAY_TYPE",
+        "FIELDNAM",
        "FILLVAL",
        "FORMAT",
+        "LABLAXIS",
+        "UNITS",


Yay for alphabetical order!

imap_processing/cdf/tests/test_imap_cdf_manager.py

anamanica added enhancement New feature or request Repo: Testing Related to testing Student Tasks suitable for student work CDF Related to CDF files labels Jun 17, 2024

anamanica requested a review from maxinelasp June 17, 2024 20:07

anamanica changed the title ~~Cdf attribute tests~~ [WIP] Cdf attribute tests Jun 17, 2024

maxinelasp reviewed Jun 18, 2024

View reviewed changes

anamanica self-assigned this Jun 18, 2024

anamanica requested review from bourque, greglucas and maxinelasp June 18, 2024 23:19

anamanica changed the title ~~[WIP] Cdf attribute tests~~ Cdf attribute tests Jun 18, 2024

anamanica marked this pull request as ready for review June 18, 2024 23:20

maxinelasp reviewed Jun 20, 2024

View reviewed changes

maxinelasp reviewed Jun 21, 2024

View reviewed changes

maxinelasp approved these changes Jun 21, 2024

View reviewed changes

bourque approved these changes Jun 24, 2024

View reviewed changes

anamanica added 12 commits June 25, 2024 13:48

Writing initial TODO comments.

24cc329

Troubleshooting

f7d0262

Schema tests passed

484d838

I think I understand instrument_id

56389b2

Fixing test file

af3f48f

Var attribute tests

64a3672

First draft

d336f90

Deleting file

6258bca

Test

34c7a98

Fixing pulled errors

fbceece

Quick

5348fb7

Trying different things to pass pre-checks

8dc2b82

anamanica added 27 commits June 25, 2024 13:48

TEST

9e14421

Fixing failed pre-check

b1c5209

Test2

2423921

Fixing pre-commit issues

4e1990f

Fixing prechecks

79f3ba9

Removing test statements

a6426c0

Moving files

6936749

Fixing PR draft comments

33bd956

Adding additional tests

4c3c4f2

Breaking up test functions

4f6e3fb

Adding more tests

9e43ad8

Getting closer to final draft

3d797d1

Finishing touches

0bb6b88

Codcov check

d2fe904

Codcov check

4045f94

Else Testing

84b78a9

Hopefully done

c454d3c

Fixing PR comments

be0d064

More work done

f446db3

Cleaning up

67271cc

Figured out Path issue

0bff645

Final touches (again)

e1644bf

Comment mods

1029e2d

Pre-commit check

db68128

Pre-commit numpydoc

53cbb04

Pre

da12644

Final Final Commit

4a7712a

anamanica force-pushed the cdf-attribute-tests branch from 951d0dc to 4a7712a Compare June 25, 2024 20:00

anamanica merged commit d012a9b into IMAP-Science-Operations-Center:dev Jun 25, 2024
16 of 17 checks passed

anamanica deleted the cdf-attribute-tests branch June 25, 2024 20:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cdf attribute tests #637

Cdf attribute tests #637

anamanica commented Jun 17, 2024 •

edited

Loading

maxinelasp left a comment

maxinelasp Jun 18, 2024

maxinelasp Jun 18, 2024

maxinelasp Jun 18, 2024

maxinelasp Jun 18, 2024

anamanica Jun 18, 2024

maxinelasp Jun 18, 2024

maxinelasp Jun 18, 2024

maxinelasp Jun 18, 2024

maxinelasp Jun 18, 2024

maxinelasp Jun 18, 2024

maxinelasp left a comment

maxinelasp Jun 20, 2024

anamanica Jun 20, 2024

maxinelasp Jun 21, 2024

anamanica Jun 21, 2024

maxinelasp Jun 20, 2024

anamanica Jun 20, 2024

maxinelasp Jun 20, 2024

maxinelasp Jun 20, 2024

anamanica Jun 21, 2024

maxinelasp Jun 21, 2024

anamanica Jun 21, 2024

maxinelasp left a comment

maxinelasp Jun 21, 2024

maxinelasp Jun 21, 2024

anamanica Jun 21, 2024

maxinelasp left a comment

maxinelasp Jun 21, 2024

bourque left a comment

bourque Jun 20, 2024

bourque Jun 24, 2024

bourque Jun 24, 2024

		Logical_source_description: IMAP Mission MAGi Normal Rate Instrument Level-1A Data.


		imap_test_section_5:

		cdf_manager = CdfAttributeManager(Path(__file__).parent.parent / "config")
		# Test that default information was loaded in from "imap_default_global_cdf_attrs.yaml"

Cdf attribute tests #637

Cdf attribute tests #637

Conversation

anamanica commented Jun 17, 2024 • edited Loading

Change Summary

Overview

New Dependencies

New Files

Deleted Files

Updated Files

Testing

maxinelasp 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

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

maxinelasp 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

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

maxinelasp 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

maxinelasp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bourque 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

anamanica commented Jun 17, 2024 •

edited

Loading