without short manes of instrument and platform//two new unittests are … #80

opsdep · 2020-03-24T20:51:09Z

…added

akorosov

Good job!
But we still have unique_together in the Source model. I guess that should stay like it is. But we have to test if it is working. That's why I suggested to add one more test.
Please add this test and fix small things here and there.

akorosov · 2020-03-25T06:47:58Z

geospaas/catalog/tests.py

@@ -206,6 +206,37 @@ def test_source(self):
        i = Instrument.objects.get(short_name='MODIS')
        source = Source(platform=p, instrument=i)
        source.save()
+
+    def test_source_without_short_names(self):
+        p = Platform.objects.get(pk=796)


It would be better not to add new entries to vocabularies fixtures and rather to use existing entries with empty short_name, long_name, etc.

akorosov · 2020-03-25T06:51:10Z

geospaas/catalog/tests.py

+        self.assertEqual(source.instrument.category, "category_without_short_name")
+
+    def test_source_empty_without_short_names(self):
+        Platform2=Platform(category = '',


Please use conventional for variable naming: variable names start from block letters. Class names from capital. Read more here:
http://www.python.org/dev/peps/pep-0008/
And here:
https://nansat.readthedocs.io/en/latest/source/conventions.html

akorosov · 2020-03-25T06:57:42Z

geospaas/catalog/tests.py

+        self.assertEqual(source2.platform.long_name, "")
+        self.assertEqual(source2.platform.series_entity, "")
+        self.assertEqual(source2.instrument.long_name, "")
+        self.assertEqual(source2.instrument.category, "")


It is important to have one more test :

Use get_or_create to add one Source pointing to one Instrument and one Latform with short names.
Use get_or_create to add exactly the same source. Test assert that create equals False in this case and source2 is equal to source 1.

Use get_or_create to add Source with the same Instrument but a different Platform (also with empty short name). Test assert that in that case create equals True and source 3 is not equal source 1.

akorosov · 2020-03-25T07:00:00Z

geospaas/vocabularies/fixtures/vocabularies.json

@@ -17483,6 +17483,18 @@
  "model": "vocabularies.instrument",
  "pk": 1457
 },
+{


This is not needed. We have already enough instruments and platforms with empty fields in fixtures.

akorosov · 2020-03-25T07:01:30Z

geospaas/vocabularies/managers.py

@@ -119,8 +119,8 @@ class PlatformManager(VocabularyManager):
                    short_name='Short_Name',
                    long_name='Long_Name')

-    def get_by_natural_key(self, short_name):
-        return self.get(short_name=short_name)
+    #def get_by_natural_key(self, short_name):


It is better to remove completely rather than comment out. We wills still have it in git history. And the code remains cleaner without.

akorosov · 2020-03-25T07:07:39Z

One minor comment. Please use conventions for branch naming as well next time. It could have been named e.g. issue1-natural-key, or something similar.

… from get_or_create method

opsdep · 2020-03-26T13:54:47Z

@akorosov
Dear Anton, please review the third test as well. I have modified the tests and return the fixture to their original state based on your comments.

akorosov · 2020-03-26T14:57:38Z

Yeah, test is good. Please also change other things.

…d naming

…ndard naming

opsdep · 2020-03-27T14:04:31Z

@akorosov
now it is ready to be merged! I have modified them based on your suggestions.

akorosov · 2020-03-27T14:34:47Z

Not yet.
Look carefully at the pending comments. E.g. check lines:
source,_ = Source.objects.get_or_create(platform=p, instrument=i)
and
i2 = Instrument.objects.get(pk=136)# "short_name": "SCATTEROMETERS"

Next time it would be easier if you click on 'Commit suggestion' so that it disappears from the discussion above.

akorosov · 2020-03-26T14:54:36Z

geospaas/catalog/tests.py

+        source2, created = Source.objects.get_or_create(platform=p, instrument=i)
+        self.assertEqual(created, 0)
+        self.assertEqual(source2, source)
+        i2 = Instrument.objects.get(pk=136)# "short_name": "SCATTEROMETERS"


Here short_name should also be empty. But it should be instrument other than 139.

akorosov · 2020-03-26T14:56:07Z

geospaas/catalog/tests.py

+        i = Instrument.objects.get(pk=139)# "short_name": ""
+        source,_ = Source.objects.get_or_create(platform=p, instrument=i)
+        source2, created = Source.objects.get_or_create(platform=p, instrument=i)
+        self.assertEqual(created, 0)


Suggested change

self.assertEqual(created, 0)

self.assertFalse(created2)

akorosov · 2020-03-26T14:56:21Z

geospaas/catalog/tests.py

+        p = Platform.objects.get(pk=661) # "short_name": ""
+        i = Instrument.objects.get(pk=139)# "short_name": ""
+        source,_ = Source.objects.get_or_create(platform=p, instrument=i)
+        source2, created = Source.objects.get_or_create(platform=p, instrument=i)


Suggested change

source2, created = Source.objects.get_or_create(platform=p, instrument=i)

source2, created2 = Source.objects.get_or_create(platform=p, instrument=i)

akorosov · 2020-03-26T14:57:04Z

geospaas/catalog/tests.py

+
+        p = Platform.objects.get(pk=661) # "short_name": ""
+        i = Instrument.objects.get(pk=139)# "short_name": ""
+        source,_ = Source.objects.get_or_create(platform=p, instrument=i)


Suggested change

source,_ = Source.objects.get_or_create(platform=p, instrument=i)

source,create1 = Source.objects.get_or_create(platform=p, instrument=i)

self.assertTrue(create1)

akorosov · 2020-03-27T12:43:00Z

geospaas/catalog/tests.py

+    def test_empty_short_names(self):
+        ''' creating objects without short_name and creating source 
+        based on them'''
+        Platform2=Platform(category = '',


Suggested change

Platform2=Platform(category = '',

platform2=Platform(category = '',

opsdep · 2020-03-30T11:29:42Z

@akorosov
Dear anton, please check it once again! :) 77b7905

akorosov · 2020-03-30T11:33:17Z

Great job, thank you @aanersc !

…ndard naming

without shortmanes of instrument and platform//two new unittests are …

c5cfcca

…added

opsdep requested a review from akorosov March 24, 2020 20:51

akorosov requested changes Mar 25, 2020

View reviewed changes

#1 the third test for investigating the uniqueness of source is added…

7aecf5b

… from get_or_create method

opsdep requested a review from akorosov March 26, 2020 13:55

Your Name added 3 commits March 27, 2020 14:42

the name of variable are modified in the tests in order to be standar…

4e90168

…d naming

#80 the name of variable are modified in the tests in order to be sta…

fc59313

…ndard naming

#80 the name of variable are modified in the tests in order to be sta…

9c9b085

…ndard naming

akorosov requested changes Mar 30, 2020

View reviewed changes

#80 assertFlase is also added and the names are chagned based on pep

77b7905

akorosov approved these changes Mar 30, 2020

View reviewed changes

akorosov merged commit d217002 into master Mar 30, 2020

akorosov pushed a commit that referenced this pull request Mar 30, 2020

#80 the name of variable are modified in the tests in order to be sta…

d642454

…ndard naming

akorosov pushed a commit that referenced this pull request Mar 30, 2020

#80 the name of variable are modified in the tests in order to be sta…

df96fbd

…ndard naming

akorosov deleted the fix_issue_1 branch March 30, 2020 11:33

This was referenced Mar 30, 2020

Issue1 gcmd short names #52

Closed

platform.short_name and instrument.short_name cannot be empty #1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

without short manes of instrument and platform//two new unittests are … #80

without short manes of instrument and platform//two new unittests are … #80

opsdep commented Mar 24, 2020

akorosov left a comment

akorosov Mar 25, 2020

akorosov Mar 25, 2020

akorosov Mar 25, 2020

akorosov Mar 25, 2020 •

edited

Loading

akorosov Mar 25, 2020

akorosov commented Mar 25, 2020

opsdep commented Mar 26, 2020

akorosov commented Mar 26, 2020

opsdep commented Mar 27, 2020

akorosov commented Mar 27, 2020

akorosov Mar 26, 2020

akorosov Mar 26, 2020

akorosov Mar 26, 2020

akorosov Mar 26, 2020

akorosov Mar 27, 2020

opsdep commented Mar 30, 2020

akorosov commented Mar 30, 2020

	source2, created = Source.objects.get_or_create(platform=p, instrument=i)
	source2, created2 = Source.objects.get_or_create(platform=p, instrument=i)

	source,_ = Source.objects.get_or_create(platform=p, instrument=i)
	source,create1 = Source.objects.get_or_create(platform=p, instrument=i)
	self.assertTrue(create1)

	Platform2=Platform(category = '',
	platform2=Platform(category = '',

without short manes of instrument and platform//two new unittests are … #80

without short manes of instrument and platform//two new unittests are … #80

Conversation

opsdep commented Mar 24, 2020

akorosov 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

akorosov Mar 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akorosov commented Mar 25, 2020

opsdep commented Mar 26, 2020

akorosov commented Mar 26, 2020

opsdep commented Mar 27, 2020

akorosov commented Mar 27, 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

opsdep commented Mar 30, 2020

akorosov commented Mar 30, 2020

akorosov Mar 25, 2020 •

edited

Loading