Skip to main content

[Fwd: Re: Please review: fix for issue #100]

1 reply [Last post]
Anonymous

Reply viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.
Alexander Alexeev

Hi Dmitri,

I'm OK with changes. Just one comment - I think commented code should be
deleted. Then you can commit.

Thanks,
Alexander

Dmitri Trounine > wrote:
> Hi Alexander,
>
> I've implemented changes that we discussed in previous messages. Please
> review:
>
> http://fisheye4.cenqua.com/changelog/cqme/?cs=735
>
> Thanks,
> Dmitri.
>
> Dmitri Trounine > wrote:
>> Alexander Alexeev wrote:
>>> As I can see you adds only sources from src/share/classes dirs. How
>>> test's sources are added? Or are they always provided through
>>> "source" tag of test's description?
>> I should explain this stuff with sources:
>>
>> - Our aim is to provide a Java source file for each class file
>> contained in the exported JAR file (except the cases like A$B.class
>> and similar).
>> - I divide all classes into four groups:
>> 1. Classes for agent and client. They come in JAR files from ME
>> Framework.
>> 2. Classes for test suite specific libraries. They come in JAR
>> files from test suites.
>> 3. Test classes. They come from separate class files listed in
>> testClasses.lst.
>> 4. Test library classes. They also come from separate class
>> files and are also listed in testClasses.lst.
>> - How the classes are added to test JAR bundles?
>> - Classes 1 and 2 are added to test JAR bundles via
>> JarBuilder.addJarFileContent() method.
>> - Classes 3 and 4 are added to test JAR bundles via
>> JarBuilder.addByteArray(s) methods invoked from within
>> JarBuilder.addTest() method.
>> - Where are located the source?
>> - In case 1, sources are in the ME Framework src/share/classes
>> directory.
>> - In case 2, sources are located in the test suite
>> src/share/classes directory.
>> - In case 3, sources are located near the test description in
>> the tree under the test suite 'tests' directory. These sources are
>> listed in test descriptions.
>> - In case 4, sources are located in the test suite
>> src/share/classes/ directory.
>>
>> How these cases are handled:
>> - Case 1: JarBuilder.addJarFileContent() is overridden in
>> MidExportBuilder. New implementation reads the content of added JAR
>> file and looks for sources in ME Framework 'src/share/classes' directory.
>> - Case 2: Same as in case 1, but the test suite
>> 'src/share/classes' directory is scanned.
>> - Case 3: Easy. ExportBundler reads the test descriptions and
>> just copies the sources.
>> - Case 4: JarBuilder.addByteArray(s) methods are overridden in
>> MidExportBuilder so that they looks for sources in the test suite
>> 'src/share/classes' directory.
>>
>> The issue #100 is about cases 2 and 4:
>> - MidExportBuilder.getSourceForClass() did only scan the ME
>> Framework 'src/share/classes' directory. So, the case 2 was not
>> properly handled if ME Framework and the test suite
>> 'src/share/classes' directories are distinct. This is what you noticed
>> in your previous comments.
>> - Case 4 was not implemented at all.
>>
>> In order to fix issue #100 we have to fix handling of case 2 and
>> add handling of case 4.
>>>
>>>>> The next comment about the name of private method add(name,
>>>>> content). This name
>>>>> is a bit of confusing. It doesn't create "name" entry with
>>>>> specified "content".
>>>>> It just tries to find source corresponding to the "name". So may be
>>>>> call it
>>>>> something like "tryToAddSource" and remove second argument.
>>>>>
>>>> I renamed it to registerClass(...) just because there is another
>>>> method registerTestDescription(...) which does the same: collects
>>>> information. I do not like name tryToAddSources because it's too
>>>> specific. One little change in code in future will made such name
>>>> meaningless.
>>>>
>>> It's really too specific and this is intentional. The method should have
>>> *self-explained* name. Now you have the method with very common name
>>> and with
>>> irrelevant set of arguments. You even had to provide comments to the
>>> code to
>>> explain what the code do. I'm really prefer the code that could be easy
>>> understood without reading comments. I'm not saying comments are bad.
>>> No, comments are *good* but the code that requires comments is bad
>>> very often.
>>>
>> Agreed. Changing the name of this methof to tryToAddSource.
>>>>> Yet another comment. Why you are overriding addByteArray() and
>>>>> addByteArrays()?
>>>>> As I understand these methods is intended to create new entries in
>>>>> the jar file
>>>>> with the specified content and the names may not correspond to real
>>>>> file names
>>>>> in the file system. I think more safe to retrieve names using
>>>>> getTestFiles(). Or
>>>>> I've missed something and addByteArray(s)() is used to add some
>>>>> class files are
>>>>> not listed in the getTestFiles()?
>>>>
>>>> Yeah, may be it's not an absolutely neccessary to use addXXX()
>>>> methods for this purpose. It's just my way to think about
>>>> JarBuilder. Invocation of addXXX() methods is an event that we
>>>> should handle. When one of these method is invoked, we think:
>>>> something is about to be added to the JAR file and we should take
>>>> care of this. In such way we are able to handle all and won't miss
>>>> anything.
>>>>
>>>> About filenames... When a class is added, it should be in a class
>>>> file. And the name of this class file should reflect the pacakage
>>>> name of the class. The things work like that in Java. So, if it's
>>>> not a class file, it's a really strange thing and the Test Suite
>>>> developers should take care of this by extending MidExportBuilder
>>>> and overriding addXXX method so that their content is being handled
>>>> in proper way. And if it's a class file we just look for it's Java
>>>> source file.
>>>>
>>> Yes, in this particular case it will work but it looks like a hack.
>>> But why to
>>> use a hack if we can provide more clear solution?
>> Since the test descriptions don't list *all* sources for test, all
>> that follows from that is hack! :)
>>> getTestFiles() should provide
>>> all the classes are needed for test execution on a device side. I
>>> think it is
>>> exactly that you need.
>>>
>> Ok, let change it as you proposed. Indeed, getTestFiles() should do
>> the same thing. Note: in my terms it's about the case 4.
>>
>>> And I've started to view ExportBundler. Seems testSourcesMap variable
>>> and
>>> getSources() method are useless since method put() for the map is
>>> called only in
>>> two places: in getSources() where empty list is added and in
>>> registerTestSource() which is never used. Or I've missed something?
>>>
>>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net