Hi Mihai, > 2. Also, how do we do code reviews? Do we use some tool, like fisheye > or github, or just the IDE? I didn't find a wiki page about that.
Unfortunately, even though we have Fishyey and Criduble installed, the later is not part of the open source offer, that was what I was told when I requested it. Frédéric THOMAS > From: mihai.ch...@gmail.com > Date: Tue, 22 Jul 2014 18:18:50 +0100 > Subject: Re: [JIRA](ISSUE #FLEX-34119) Dev review request > To: dev@flex.apache.org > > Hi Alex, > > > thanks for taking a look. I was also surprised to discover not only > that the parentTable variable wasn't necessary, but also that two > entire if branches (for add and remove) *always* ended up throwing the > 'Bookmark no longer valid' error (see the commit message here[1] for > details). In other words, they had never been tested. > > If there is some edge case, when we discover it we'll add a unit test > to make sure it doesn't return. > Speaking of which, I've just added the setItemAt operation to the unit > test, and I got some more 'Bookmark no longer valid' errors. I'll fix > these new occurrences hopefully tomorrow. Once I do and once I run the > mustella tests, I'll ask you to take another look before I merge with > develop. > > Two questions: > > 1. Would it be very complicated to run unit tests automatically in the > ant build script, or to the server CI process? > 2. Also, how do we do code reviews? Do we use some tool, like fisheye > or github, or just the IDE? I didn't find a wiki page about that. > > > Thanks, > Mihai > > > [1] > https://fisheye6.atlassian.com/changelog/flex-sdk?cs=4cb80bfbb37225b86b9e69cc4ec2fc21efe0a668 > > On 22 July 2014 17:49, Alex Harui <aha...@adobe.com> wrote: > > Hi Mihai, > > > > Thanks for taking on such a challenging goal. > > > > I don't see anything obviously wrong. IMO, the best thing to do would be > > to set up mustella and run the Advanced DataGrid tests. That's what our > > CI system will do when you finally check this into the develop branch. > > > > It is a bit puzzling that the old code messed around with the parent stack > > and it looks like you no longer need to. I wonder if there is some edge > > case associated with that. > > > > -Alex > > > > On 7/22/14 4:00 AM, "Mihai Chira" <mihai.ch...@gmail.com> wrote: > > > >>On the branch "FLEX-34119" I committed two unit tests and a fix for > >>this bug. If anyone would like to take a look, and run the tests, it > >>would be really helpful, because it's a trickier area of the SDK. > >>Could you let me know if: > >>* you think the unit tests are in the right folder > >>* the unit tests run as expected (see the README in > >>HierarchicalCollectionViewCursor_FLEX_34119_Test for some explanations > >>about it) > >>* it looks like the commits might break anything else > >>* you think the variable renames in HierarchicalCollectionViewCursor are > >>helpful > >>* we could run the unit tests automatically on each ant build. Is that > >>possible now? > >> > >>I'm still adding an operation to the unit test (setItemAt), and > >>looking to remove some duplicated code from > >>HierarchicalCollectionViewCursor.collectionChangeHandler. > >>I also plan to run the mustella tests soon, to make sure I haven't > >>broken anything. > >> > >> > >>Thank you! > >>Mihai > >
