New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adds function on colormap objects to set RGB values #1553

Merged
merged 5 commits into from Sep 25, 2015

Conversation

Projects
None yet
3 participants
@doutriaux1
Member

doutriaux1 commented Sep 18, 2015

fix #1484

doutriaux1 added some commits Sep 18, 2015

@@ -0,0 +1,10 @@
import vcs

This comment has been minimized.

@aashish24

aashish24 Sep 19, 2015

Contributor

Instead of making two tests.. can you just make one? We have lot many tests and now we need to make sure that we don't grow the number as much

This comment has been minimized.

@doutriaux1

doutriaux1 Sep 19, 2015

Member

I like one test per issue. So we know exactly what broke.

This comment has been minimized.

@aashish24

aashish24 Sep 22, 2015

Contributor

@doutriaux1 like I said, we are having too many tests. In this case, I think you can easily put both of them together into one as they are testing getter and setters.

This comment has been minimized.

@doutriaux1

doutriaux1 Sep 22, 2015

Member

no, we have established a policy of one test per feature. It takes 2 seconds at most to run, this will save one second.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Sep 22, 2015

per feature not per getter and setter. Also, @williams13 bought this issue as well. Even if you are writing unit tests, you can include them in one file vs having each into own separate file. The main thing is you are testing the code (the lines). Having a very large number of test files is not good either.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Sep 22, 2015

I am just suggesting best software practices. I understanding if you are testing a feature (math, model of some sort etc) then yes, having individual tests help.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Sep 22, 2015

@jbeezley paranthesis closed in add_test too early. Pushing a fix now.

@jbeezley

This comment has been minimized.

Contributor

jbeezley commented Sep 22, 2015

In this particular case, I think the tests could be combined into one, but in general I don't think there is such a thing as "too many tests" in any practical sense. There is a point at which testing can become too slow, but I would argue it being a result of either a bad unit testing framework (i.e. lots of setup code, not mocking process intensive components) or a fundamental problem with software itself (i.e. orthogonal components cannot be tested independently).

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Sep 22, 2015

@aashish24 go ahead if you have time, push an update to the branch.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Sep 22, 2015

In this particular case, I think the tests could be combined into one, but in general I don't think there is such a thing as "too many tests" in any practical sense. There is a point at which testing can become too slow, but I would argue it being a result of either a bad unit testing framework (i.e. lots of setup code, not mocking process intensive components) or a fundamental problem with software itself (i.e. orthogonal components cannot be tested independently).

@jbeezley I agree with most of you said. Like I said earlier, unnecessarily increasing number of test files is not a good practice unless required. If we can combine tests into one file that would be great. This is particularly true for unit tests which is pretty much what we have in this branch. More testing is good though. I didn't mean that. We should continue to add more tests to increase code coverage.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Sep 22, 2015

Will do later today @doutriaux1 thanks.

@jbeezley

This comment has been minimized.

Contributor

jbeezley commented Sep 22, 2015

I don't think the number of test files is a useful metric for anything. My point is that, assuming identical coverage, more tests is better than fewer tests.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Sep 22, 2015

Agree @jbeezley 👍 We are on the same page.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Sep 22, 2015

@aashish24 so you are on the same page as @jbeezley two files are better than one 😉

I don't think the number of test files is a useful metric for anything. My point is that, assuming identical coverage, more tests is better than fewer tests.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Sep 23, 2015

In this particular case, I think the tests could be combined into one

Jon said that in this particular case one is better (I was saying the same). One is better in this case, in some other cases two or more is desired.

@aashish24 aashish24 force-pushed the issue_1484_colormap_setcolorcell branch from a345ffa to 2bf875a Sep 24, 2015

Merge remote-tracking branch 'origin/master' into issue_1484_colormap…
…_setcolorcell

Conflicts:
	testing/vcs/CMakeLists.txt
@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Sep 25, 2015

@aashish24 any idea what happened on crunchy?

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Sep 25, 2015

@doutriaux1 no idea. Its only one test failing on crunchy... I think the changes are safe enough so I am willing to merge it.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Sep 25, 2015

LGTM 👍

aashish24 added a commit that referenced this pull request Sep 25, 2015

Merge pull request #1553 from UV-CDAT/issue_1484_colormap_setcolorcell
adds function on colormap objects to set RGB values

@aashish24 aashish24 merged commit 3fa4d27 into master Sep 25, 2015

5 of 9 checks passed

cont-int/LLNL/Darwin-Mac 10.10.3 running 'ctest -j4 -D Experimental' (Thu Sep 24 17:02:28 2015)
Details
cont-int/LLNL/Linux-annie Ubuntu 15.04 (FULL/MESA) running 'ctest -j15 -D Experimental' (Thu Sep 24 15:30:32 2015)
Details
cont-int/LLNL/Linux-crunchy RH6 (FULL) running 'ctest -j12 -D Experimental' (Thu Sep 24 14:43:10 2015)
Details
continuous-integration/kitware-buildbot/uvcdat-garant-linux-release/ Build done.
Details
cont-int/LLNL/Darwin-Mac1 10.10.5 (NOGUI) running 'rm -rf /Users/doutriaux1/bot/build' (Thu Sep 24 14:00:29 2015)
Details
cont-int/LLNL/Darwin-Mac2 10.10.5 (FULL) running 'rm -rf /Users/doutriaux1/bot/build' (Thu Sep 24 14:00:31 2015)
Details
cont-int/LLNL/Linux-oceanonly RH6 (MESA/NOGUI) running 'ctest -j12 -D Experimental' (Thu Sep 24 14:35:23 2015)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@aashish24 aashish24 deleted the issue_1484_colormap_setcolorcell branch Sep 25, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment