flex-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Frédéric THOMAS <webdoubl...@hotmail.com>
Subject RE: [JIRA](ISSUE #FLEX-34119) Dev review request
Date Fri, 08 Aug 2014 16:32:18 GMT
For Crucible (code review), nothing we can do on our side, only INFRA could request it and
I didn't ask them to do so (if you want to ask, it's up to you, it would be a very nice feature
but you don't be in hurry), only FishEye has been setup while ago https://fisheye6.atlassian.com/browse/flex-sdk
Frédéric THOMAS

> From: mihai.chira@gmail.com
> Date: Fri, 8 Aug 2014 17:21:04 +0100
> Subject: Re: [JIRA](ISSUE #FLEX-34119) Dev review request
> CC: dev@flex.apache.org
> 
> Frédéric, have you found out anything else regarding FishEye? I took a
> look, but the Flex project isn't there (in fact, there seems to be
> only one project, called "Default Project". It would be great to
> review each other's code using this.
> 
> Piotr, have you committed the TLF automatic test running code yet? It
> would be great to run the test automatically in flex too, not least
> because while trying to solve FLEX-34119 I had to create five separate
> unit tests.
> 
> Alex, I think I've finally nailed FLEX-34119! Since I first wrote
> about it, it morphed into a total of five bugs: FLEX-34440,
> FLEX-34424, FLEX-34456, FLEX-34458, and the original FLEX-34119. All
> of them have unit tests, and now they all pass, which took a while.
> (Wow, I have to say, HierarchicalCollectionViewCursor was pretty
> broken.)
> 
> There is a serious amount of code now on the FLEX-34119 branch[1]. It
> would really help if you (and potentially others) could take another
> look when you have time. Each commit has extensive information for
> what was wrong, and what the solution is. Let me know if you think any
> of those bugs could be solved in other ways. (Note that there are 7 or
> 8 commits which are not mine; they were introduced when I merged with
> develop.)
> 
> 
> [1] https://fisheye6.atlassian.com/graph/flex-sdk#branch=FLEX-34119&hl=Lineage&scrollToCsid=82d6b51694e846f0fb4e5505bf5a1b0601c21e7b
> 
> On 22 July 2014 21:34, Alex Harui <aharui@adobe.com> wrote:
> > Regarding code review, we do have tools but for most changes, if the tests pass
you check it in and folks review the commit email.
> > Sent via the PANTECH Discover, an AT&T 4G LTE smartphone.
> >
> > Mihai Chira <mihai.chira@gmail.com> wrote:
> >
> >
> > 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 <aharui@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.chira@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
> >>
 		 	   		  
Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message