Code reviews for the recent "Test Export" updates
Hi Dmitri, team,
Let's devote this thread to the code reviews for the latest updates
for "Test Export" feature.
Let's discuss first what exactly should be reviewed, there were lots
of changes, in various branches, and even multiple bugfixes in the
single branches, with some merges between the branches.
Frankly, I'm lost a little bit.
Dmitri, you've done A LOT :) Let's sort it out and make it easier to
Typically, I'd suggest to create a separate branch for every bugfix,
and make the separated bugfixes as independent from each other as
possible. I understand that this is not always possible.
But it is pretty hard for everybody, to have multiple bugfixes in the
At the moment, my understanding is that there are 4 different issues
that were (are being?) fixed:
- Export of distributed tests
- Issue #60: In Test Export mode, not all copied sources are listed.
- Issue #57: Test Export creates and uses src.map file
- Improved export of test sources (or is this part of ISSUE #60?)
- Issue #38: Export directory is overwritten without warnings
Am I missing something, were there any other bugfixes?
I took a look at the some changes in the Dmitri's branch, but there
are so many of them, and I'm afraid that I could spend quite some
time trying to review something that's not needed to be reviewed...
Dmitri, would it be possible for you to create independent branches
for every bugfix, with clear bug id associated with every change, with
bugs' evaluations updated with information about the bugfix (what was
done and why), and post the relevant info into this thread?
I'm really sorry, that's a bit of extra work for you. Let me know if
the request is not feasible.
>From my side, I promise to devote one full day reviewing all your
changes, once you provide the list of bugfixes, with updated bug
reports, with separated branches.
Given the amount of the code changes, I think that there should be
some unit tests provided. Some time ago we discussed unit tests policy
and my understanding is that we should strive to provide unit tests
for every new feature, especially for such big changes.
(Oh, I just noticed that you do provide some unit test, for new parser
functionality. But do we need to provide some more unit tests for all
Also, please note that some areas must be reviewed by Stan, and we
cannot commit them without his approval (packages com.sun.tck.cldc and
com.sun.cldc). In your case, TestBundler must definitely be reviewed
by Stan. The sooner you submit such CLDC-related changes for his
review, the sooner you get the response.. :)
P.S. With big updates, it's always hard to properly review them, so it
would be great to find a balance somehow, and provide changes in more
iterative process, with a list of smaller fixes rather then with one
huge change. But again, it's not always easy to figure this out.
To unsubscribe, e-mail: email@example.com
For additional commands, e-mail: firstname.lastname@example.org