Feature enhancement#118 times unit test #192

nip5 · 2018-06-13T20:49:13Z

Merge into master unit test for Times.c function interpolate_monthlyValues.

…_temp.cc

…_Enhancement#118_TimesUnitTests

…ement#115_SW_SoilWater_UnitTests

…nt#115_SW_SoilWater_UnitTests

…ub.com/DrylandEcology/SOILWAT2 into feature_Enhancement#118_TimesUnitTests

dschlaep · 2018-07-10T19:52:25Z

test/test_Times.cc

+    interpolate -> cloudcov[11] = 12;
+    interpolate_monthlyValues(interpolate -> cloudcov, interpolate -> cloudcov_daily);
+    EXPECT_DOUBLE_EQ(interpolate -> cloudcov_daily[365], 16.129032258064516);
+


Please add an expectation for MAX_DAYS

Oh yes, I forgot about the leap year!

dschlaep · 2018-07-10T19:57:25Z

This PR references to an updated commit of the googletest submodule. Please don't do that without accounting for #206 (particularly as addressed by commit 5baeec0)

nip5 · 2018-07-10T21:14:08Z

How should I address #206 here? Checkout googletest one commit before this problem was introduced as google/googletest@87a4cdd does?

dschlaep · 2018-07-11T10:10:02Z

cherry pick 5baeec0 for now
don't update the googletest submodule in the future without specific efforts and dedicated commit

nip5 · 2018-07-11T15:09:38Z

I didn't make any efforts to update it on my branch, the only way I see that it could have been updated is when I merged in master to my branch. By cherry pick do you mean just do what you did in that commit, replace my googletest directory and make the change in .gitmodules?

dschlaep · 2018-07-11T15:16:31Z

https://www.git-scm.com/docs/git-cherry-pick

dschlaep · 2018-07-11T15:48:17Z

I just had the same failure at PR #209 and opened a new issue for this (#210) -- until #210 is fixed, you need to manually restart the build on appveyor.

nip5 · 2018-07-11T16:03:11Z

How can that be done without doing a new commit? I have been able to find that is possible but no reliable source saying how to do it, the best I've found is "there is a button"; I don't see a button.

…nt#115_SW_SoilWater_UnitTests

codecov · 2018-07-25T21:08:06Z

Codecov Report

Merging #192 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #192      +/-   ##
==========================================
+ Coverage   60.68%   60.71%   +0.02%     
==========================================
  Files          19       19              
  Lines        3584     3584              
==========================================
+ Hits         2175     2176       +1     
+ Misses       1409     1408       -1

Impacted Files	Coverage Δ
Times.c	`45.32% <100%> (ø)`	⬆️
SW_SoilWater.c	`55.2% <100%> (ø)`	⬆️
rands.c	`48.02% <0%> (+0.65%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6684d64...833cc72. Read the comment docs.

nip5 · 2018-07-25T21:14:03Z

@CaitlinA This branch is ready for review.

CaitlinA

Hi Nathan,

I am only address the changes to the Times.c files here. I imagine there are differences in test_SW_Soilwater.cc on your other branch.

I only have comments about comments, not any of the test themselves.

The googletest folder changes, is this because the master updated the googletest submodule and you pulled down these changes to your branch?

CaitlinA · 2018-07-26T17:15:32Z

test/test_Times.cc

+    SW_SKY SW_Sky;
+    SW_SKY *interpolate = &SW_Sky;
+    unsigned int i;
+    // set all monthlyValues all 10


// set all monthly values to 10?

Yes, since the test relies on the values here, I need to know exactly what the values are in order to test properly. Setting them all to 10 ensures this. I will add this to the comment to be more clear.

CaitlinA · 2018-07-26T17:17:46Z

test/test_Times.cc

+    for (i = 0; i < length(interpolate -> cloudcov); i++){
+      interpolate -> cloudcov[i] = 10;
+    }
+    interpolate -> cloudcov_daily[0] = 0;


Why do you have to specifically so cloudcov_daily[0] to 0 here? Should this be part of the functionality.

I did that to illustrate that cloudcov_daily[0] does not change by setting it before the function call than testing that is still that value afterwords. The function does not modify this value so it is whatever it is before the call.

CaitlinA · 2018-07-26T17:18:51Z

test/test_Times.cc

+    Reset_SOILWAT2_after_UnitTest();
+
+    // change first value to 20 and test the changes
+    interpolate -> cloudcov[0] = 20;


Do you actually test these changes? Could you clarify your message to something like change first value to a value to test if .....

Yes, changing cloudcov[0] translates to changing the cloud cover in January, which changes the interpolation of all the days. I test these changes underneath this initialization.

CaitlinA · 2018-07-26T17:19:39Z

test/test_Times.cc

+    EXPECT_DOUBLE_EQ(interpolate -> cloudcov_daily[365], 15.161290322580644);
+    // test last day on leap year
+    EXPECT_DOUBLE_EQ(interpolate -> cloudcov_daily[MAX_DAYS], 15.483870967741936);
+    // change december monthly value to ensure meaningful final interpolation


please add more enter / space around new test conditions to clarify

Do you mean have a space for every EXPECT? Ie.
EXPECT_EQ

EXPECT_EQ

...

No - I mean when ever you change the the input conditions, add more breaks. So enter after line 61.

nip5 · 2018-07-26T17:45:30Z

Yes, the googltest file changed in master so when merging into my branch it changed mine as well.

CaitlinA · 2018-07-26T19:01:55Z

closes #118

nip5 · 2018-07-26T19:35:22Z

What needs to be done now before we can merge the pull request and delete this branch?

CaitlinA · 2018-07-26T19:39:56Z

Closes #118

nip5 added 13 commits June 1, 2018 13:11

Added testing file and made interpolate_monthlyValues testable

fce2a94

Added macro for length of array

39d3bc3

Removed got here print statements for debugging

b4a65b2

Merge branch 'master' into Enchancement#115_SW_SoilWater_UnitTests

9327536

Added more tests to cover all conditionals of interpolate_monthlyValues

ccfe28a

Increased accuracy in SWSWCbulk2SWPmatric test by calculating val in R

788f322

Merged with branch Enhancement#171 to get updates to test_SW_Flow_lib…

80364e3

…_temp.cc

Added testing for snowmelt in SWSWCSadjustSnow

75d516f

Implemented full coverage for function SW_SWCbulk2SWPmatric

edd2201

Modified unit tests to work with structure pointer and no return

c21121f

Implemented more tests for different scenarios

9ee7d98

Resolved conflicts with master

b830b95

Merge branch 'Enhancement#171_SW_VWCBulkRes_InvalidVals' into feature…

2ef7de7

…_Enhancement#118_TimesUnitTests

nip5 added the enhancement label Jun 13, 2018

nip5 added this to the Code testing milestone Jun 13, 2018

nip5 requested a review from CaitlinA June 13, 2018 20:49

nip5 added 13 commits June 13, 2018 15:23

Merge branch 'Enhancement#171_SW_VWCBulkRes_InvalidVals' into Enchanc…

32462ee

…ement#115_SW_SoilWater_UnitTests

Added test to SW_VWCBulkRes for when ideal inputs are entered

e6dec84

Changed all doubles to RealD types

15bf7ad

#

4ebfd27

Merge branch 'feature_Enhancement#118_TimesUnitTests' into Enchanceme…

1dc6951

…nt#115_SW_SoilWater_UnitTests

Fixed failing after replace

d7ef355

Replaced flow_lib_temp test

0d04dd6

reverted back to version before changing array indexing

0ae687c

Merge branch 'feature_Enhancement#118_TimesUnitTests' of https://gith…

81dd645

…ub.com/DrylandEcology/SOILWAT2 into feature_Enhancement#118_TimesUnitTests

reverted to before array indexing changes

aff4117

Merge branch 'master' into feature_Enhancement#118_TimesUnitTests

bf7569f

merged with master

cbf7124

Changed functioncomments to doxygen standards, added base1 note

875785c

nip5 mentioned this pull request Jul 10, 2018

Bug fix#195 array not updated #196

Closed

dschlaep reviewed Jul 10, 2018

View reviewed changes

nip5 added 2 commits July 10, 2018 13:27

added test for leap years

998601b

Fixed tests

d750899

cherry picked commit 5baeec0 to address issue #206

bcbc4b4

nip5 added 6 commits July 25, 2018 11:21

Merge branch 'master' into Enchancement#115_SW_SoilWater_UnitTests

acc9b44

Merge branch 'feature_Enhancement#118_TimesUnitTests' into Enchanceme…

5360de4

…nt#115_SW_SoilWater_UnitTests

remerged with branches #118 and master

54549fa

Modify code styling as per unit testing guide

cf7d963

Remerge with branch #115

91530cb

Modify code style as per unit testing guidelines

da922e4

CaitlinA requested changes Jul 26, 2018

View reviewed changes

Modify spacing and comments for clearity

833cc72

CaitlinA approved these changes Jul 26, 2018

View reviewed changes

CaitlinA closed this Jul 26, 2018

CaitlinA reopened this Jul 26, 2018

CaitlinA merged commit 9d29bdf into master Jul 26, 2018

CaitlinA deleted the feature_Enhancement#118_TimesUnitTests branch July 26, 2018 19:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature enhancement#118 times unit test #192

Feature enhancement#118 times unit test #192

nip5 commented Jun 13, 2018

dschlaep Jul 10, 2018

nip5 Jul 10, 2018

dschlaep commented Jul 10, 2018

nip5 commented Jul 10, 2018

dschlaep commented Jul 11, 2018

nip5 commented Jul 11, 2018

dschlaep commented Jul 11, 2018

dschlaep commented Jul 11, 2018

nip5 commented Jul 11, 2018

codecov bot commented Jul 25, 2018 •

edited

Loading

nip5 commented Jul 25, 2018

CaitlinA left a comment

CaitlinA Jul 26, 2018

nip5 Jul 26, 2018

CaitlinA Jul 26, 2018

nip5 Jul 26, 2018

CaitlinA Jul 26, 2018

nip5 Jul 26, 2018

CaitlinA Jul 26, 2018

nip5 Jul 26, 2018

CaitlinA Jul 26, 2018

nip5 commented Jul 26, 2018

CaitlinA commented Jul 26, 2018

nip5 commented Jul 26, 2018

CaitlinA commented Jul 26, 2018

Feature enhancement#118 times unit test #192

Feature enhancement#118 times unit test #192

Conversation

nip5 commented Jun 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschlaep commented Jul 10, 2018

nip5 commented Jul 10, 2018

dschlaep commented Jul 11, 2018

nip5 commented Jul 11, 2018

dschlaep commented Jul 11, 2018

dschlaep commented Jul 11, 2018

nip5 commented Jul 11, 2018

codecov bot commented Jul 25, 2018 • edited Loading

Codecov Report

nip5 commented Jul 25, 2018

CaitlinA 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

nip5 commented Jul 26, 2018

CaitlinA commented Jul 26, 2018

nip5 commented Jul 26, 2018

CaitlinA commented Jul 26, 2018

codecov bot commented Jul 25, 2018 •

edited

Loading