[Cdk-devel] [ cdk-Patches-3519885 ] New AtomPlacer3D test class

[SourceForge.net <no...@so...>]

Patches item #3519885, was opened at 2012-04-20 09:33
Message generated for change (Comment added) made by egonw
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3519885&group_id=20024

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: master
Group: Needs Review
Status: Open
Resolution: None
Priority: 5
Private: No
Submitted By: Daniel Szisz (danielszisz)
Assigned to: Rajarshi Guha (rajarshi)
Summary: New AtomPlacer3D test class 

Initial Comment:
Hi !

I made some new tests that tests the not-yet-tested functionalities of AtomPlacer3D. It is intended to go into master. I also post a modification patch to the
first patch. Hopefully these tests will reveal some bugs in ModelBuilder3D.

----------------------------------------------------------------------

>Comment By: Egon Willighagen (egonw)
Date: 2012-04-22 01:59

Message:
(Otherwise, great work you are doing!)

----------------------------------------------------------------------

Comment By: Egon Willighagen (egonw)
Date: 2012-04-22 01:57

Message:
Daniel, some comments:

1. can you add the copyright header?

2. in code like:

+       @Test                                                              
                                                                           
             
+       public void testAllHeavyAtomsPlaced_IAtomContainer() {             
                                                                           
             
+               AtomPlacer3D atmplacer=new AtomPlacer3D();                 
                                                                           
             
+               List<IAtomContainer> molecules = new
ArrayList<IAtomContainer>();                                               
                                    
+               molecules.add(MoleculeFactory.makeAlkane(5));              
                                                                           
             
+               molecules.add(MoleculeFactory.makeBenzene());              
                                                                           
             
+               molecules.add(MoleculeFactory.makePhenylAmine());          
                                                                           
             
+               molecules.add(MoleculeFactory.makeCyclobutadiene());       
                                                                           
             

are these four different situations, with different code run? That is, can
they fail because of different reasons? If so, I suggest to split them up
into separate unit tests...

3. I prefer longer variable names that I can give easily meaning too... s1
can be anything. For unit tests I find this less important, but just want
to mention it. Formally, var names have to be at least three characters...

4. Assert.assertTrue(anglev==109.608);

Use: assertEquals*109.608, anglev, 0.001);

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3519885&group_id=20024