From: SourceForge.net <no...@so...> - 2012-04-24 09:11:01
|
Patches item #3519885, was opened at 2012-04-20 09:33 Message generated for change (Comment added) made by danielszisz 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 Revision Status: Open Resolution: None Priority: 5 Private: No Submitted By: Daniel Szisz (danielszisz) >Assigned to: Egon Willighagen (egonw) 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: Daniel Szisz (danielszisz) Date: 2012-04-24 02:11 Message: Hello Egon ! First apply the 0001-New-AtomPlacer3DTest-class.patch patch that creates the class, and then apply the second one, the 0001-Updated-and-corrected-new-AtomPlacer3D-Test-class-by.patch, which just updates the first one with the corrections that you suggested. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2012-04-24 01:00 Message: Daniel, I cannot apply the 0001-Updated-and-corrected-new-AtomPlacer3D-Test-class-by.patch to master... it both fails if I apply it without and with the other patch... Please give instructions what patches to apply, what order (they are now both 0001), and anything else that helps... ---------------------------------------------------------------------- Comment By: Daniel Szisz (danielszisz) Date: 2012-04-23 23:59 Message: Egon, would you please accept this patch ? This patch consists of two files : the first adds the new FurtherAtomPlacer3DTest class the second updates it with the corrections you added 2 days ago. ---------------------------------------------------------------------- Comment By: Daniel Szisz (danielszisz) Date: 2012-04-23 00:33 Message: Corrected the test file based on the suggestions of Egon. ---------------------------------------------------------------------- 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 |