From: SourceForge.net <no...@so...> - 2012-05-02 15:59:18
|
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: Accepted Status: Closed Resolution: Accepted 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: Daniel Szisz (danielszisz) Date: 2012-05-02 08:59 Message: Accepted and closed. ---------------------------------------------------------------------- Comment By: Rajarshi Guha (rajarshi) Date: 2012-05-02 07:57 Message: aplied and pushed ---------------------------------------------------------------------- Comment By: Daniel Szisz (danielszisz) Date: 2012-05-02 07:55 Message: Hello Rajarshi! Yes, can u apply to the master branch please? thanks. ---------------------------------------------------------------------- Comment By: Rajarshi Guha (rajarshi) Date: 2012-05-02 05:32 Message: The commits in 3 look OK. They go into master right? ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2012-04-29 23:29 Message: Rajarshi, Daniel placed the patches online [0] which I rebased on master [1], of which I already applied three JavaDoc fixes to cdk-1.4.x and master [2]. Then I factored out patches related to the AtomPlacer3D [3] but did not get around to fully checking it.... Daniel, it would be cool if those five patches in [3] can be collapsed into one or two patches, as they basically just extend each other... but I'm more than happy if to go in as five separate ones too... Rajarshi, those patches from [3] compile, but I found it hard to see if there were regressions, because master current has a lot of JUnit errors more than earlier... [4] But, Rajarshi, if you can have a look at those 5 patches in [3] instead ? Egon 0.https://github.com/danielszisz/cdk/commits/master 1.https://github.com/egonw/cdk/commits/392-m-danielmaster 2.https://github.com/cdk/cdk/commits/cdk-1.4.x 3.https://github.com/egonw/cdk/commits/393-m-atomPlacer 4.http://pele.farmbio.uu.se/supernightly/ ---------------------------------------------------------------------- Comment By: Rajarshi Guha (rajarshi) Date: 2012-04-29 16:13 Message: The first patch (New Atom ...) applies OK, but the second one fails ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2012-04-24 02:26 Message: Daniel, sorry, it doesn't work here on a master (commit 7351e8e563287a7287965252c50f900ee2845c73): egonw@laptopEgon:~/var/Projects/SourceForge/git/cdk$ git am -3 --ignore-whitespace ~/Downloads/0001-New-AtomPlacer3DTest-class.patch Applying: New AtomPlacer3DTest class egonw@laptopEgon:~/var/Projects/SourceForge/git/cdk$ git am -3 --ignore-whitespace ~/Downloads/0001-Updated-and-corrected-new-AtomPlacer3D-Test-class-by.patch Applying: Updated and corrected new AtomPlacer3D Test class by danielszisz Using index info to reconstruct a base tree... <stdin>:10: trailing whitespace. /* Copyright (C) 2000-2012 Christoph Steinbeck, Egon Willighagen <stdin>:55: trailing whitespace. * @author danielszisz <stdin>:84: trailing whitespace. Assert.assertTrue(atmplacer.allHeavyAtomsPlaced(benzene)); <stdin>:159: space before tab in indent. molecule.getAtom(i).setFlag(CDKConstants.ISPLACED, true); <stdin>:186: trailing whitespace. atom.setFlag(CDKConstants.ISPLACED, true); warning: squelched 8 whitespace errors warning: 13 lines add whitespace errors. Falling back to patching base and 3-way merge... Auto-merging src/test/org/openscience/cdk/modeling/builder3d/FurtherAtomPlacer3DTest.java CONFLICT (content): Merge conflict in src/test/org/openscience/cdk/modeling/builder3d/FurtherAtomPlacer3DTest.java Failed to merge in the changes. Patch failed at 0001 Updated and corrected new AtomPlacer3D Test class by danielszisz When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort". Rajarshi, maybe you can give it a try? ---------------------------------------------------------------------- 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 |