Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Publish openjdk.test.jlm project into openjdk-systemtest Adopt repository #89

Merged
merged 1 commit into from
Apr 17, 2018

Conversation

Mesbah-Alam
Copy link
Contributor

@Mesbah-Alam Mesbah-Alam commented Mar 22, 2018

Publish openjdk.test.jlm project into openjdk-systemtest Adopt repository. This project can be built and run against both OpenJDK and OpenJDK-OpenJ9 SDKs.

Related Issue: #88

@smlambert
Copy link
Contributor

Thanks Mesbah, I am going to look at this over the next few days.

@Mesbah-Alam
Copy link
Contributor Author

Openj9 Issues reported via tests pertaining to this PR so far:
eclipse-openj9/openj9#1566
eclipse-openj9/openj9#1520

<!-- Projects which need to be built before this one. -->
<!-- dir must be set on the ant task otherwise the basedir property is not set to a new value in the subant task. -->
<target name="build-dependencies">
<!-- <ant antfile="${stf_root}/stf.build/build.xml" dir="${stf_root}/stf.build" inheritAll="false"/> -->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line is commented out, is it not needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this build-dependencies target. This code needs to be cleaned up. Just discovered that, the same clean up needs to happen in other openjdk-systemtest projects too, where the same commented out code remains.

We don't need to build stf as part of building each individual test project, as stf is built from openjdk.build prior to building the test projects.

i++;
}
} catch (UnsupportedOperationException uoe) {
Message.logOut("One of the operations you tried is not supported");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General comment for the logged messages in this file, that many of them do not seem very useful. Do the stacktraces or some other info get automatically logged?

AttributeNotFoundException log message "Attribute does not exist on the MBean" does not really add value. Based on how some of these tests were written, where try catch blocks around large pieces of code, that it may not be possible how it is structure to include more info (like what attribute, as there are 6 setAttribute calls in this block), but if logging is supposed to help debugging, we may have to revisit this.

Copy link
Contributor Author

@Mesbah-Alam Mesbah-Alam Mar 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. The reporting and error logging in this test is rather messy. They generate "CSV" data files where some info is logged. The rest go into standard err and out. There are also some tests (e.g. VMLogger, where a log file is used to log detailed results of what the invoked Bean API's return. The output handling is not standardized in general. However, it is quite a lot of work to be honest to standardize the reporting mechanism of these tests. So, may be we could cover it in a separate enhancement work item, as you indicated in your next comment too (just noticed). :)

this.csv.println(df.format(upTime / 1000) + "," + classCount + "," +
loadedCount + "," + unloadedCount);
} catch (ConnectException ce) {
Message.logOut("Problem with the MBean access");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as previous about the logging story applies across many test classes.

ManagementFactory.RUNTIME_MXBEAN_NAME,
RuntimeMXBean.class);
String vendor = runtimeBean.getVmVendor();
if (vendor.contains("IBM")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this true for OpenJ9, I thought the vendor name changed for that SDK, as did java -version output? We should check.

Also, I guess if it is true (and you are testing against an IBM SDK, what does this check and information do for the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the catch! It is indeed not true for OpenJ9. For OpenJ9, runtimeBean.getVMVendor() returns Eclipse OpenJ9. I will update the code to reflect this.

@smlambert
Copy link
Contributor

I recognize that this code is migrated, and so do not expect that we can address the inconsistent logging, asserting, and println approach at this time, but we may want an epic/issue at some point to consolidate it.

@Mesbah-Alam
Copy link
Contributor Author

@smlambert I created the following issue to track the work to standardize the error reporting approach of JLM tests : #94

Copy link
Contributor

@smlambert smlambert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Mesbah, I know these tests run, and have been 'soaked' at other build farms. I would not hold this up for the logging cleanup, it is more appropriate to get these in and running (barring any other comments from reviewers), to vet out any other issues, and later come back to item #94

@Mesbah-Alam Mesbah-Alam force-pushed the addJlmSystemTestToAdopt branch 2 times, most recently from 21306f1 to d839d87 Compare March 29, 2018 19:19

A full list of the java.lang.management functions can be found in the Java 6 API documentation. As mentioned above, the functions are provided by a set of Platform MBeans:
> - ClassLoadingMXBean
- CompilationMXBean
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the bullets
First item alone is seen in grey color and with different indentation where as second item onward its black color.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you have to remove that angle bracket i.e >

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is there in more than one place, remove them as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still there in 2 more places

  1. stf.pl -test=TestJlmLocal
  2. stf.pl -test=jlm_remote_test_name

- MemoryMXBean
- MemoryPoolMXBean
- OperatingSystemMXBean
- RuntimeMXBean<
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove open angle bracket above i.e <

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this still

Copy link
Contributor Author

@Mesbah-Alam Mesbah-Alam Apr 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must have pushed the wrong branch! It should be there now.


Once you have set up STF and compiled the test source in your workstation, you can run any test locally from the command line.

####LM Local Tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'J' is missing above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed.


###How are the tests implemented?

The test cases are all implemented in Java, using the *STF*. Please see [STF test framework manual](../../../stf/stf.core/docs/STF-Manual.md).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this link is broken.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

> - STF
- Testcase Material

Since JLM tests are written based on STF, they will require the same set up as STF in general. Please follow the [STF Getting Started Guide](../../../stf/stf.core/docs/STF-GettingStarted.md) and install everything that is required to run STF tests.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this link as well broken.

5. How do I know whether it worked or failed?
6. Is there anything else I should know?
7. Are there any references?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like href is not added properly as I don't see hyper links for these when opened from your branch https://github.com/Mesbah-Alam/openjdk-systemtest/blob/addJlmSystemTestToAdopt/openjdk.test.jlm/doc/jlm.md

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing in-page anchors have been added.

@Mesbah-Alam Mesbah-Alam force-pushed the addJlmSystemTestToAdopt branch 4 times, most recently from dc8bcd4 to add416e Compare April 6, 2018 14:49
@Mesbah-Alam Mesbah-Alam force-pushed the addJlmSystemTestToAdopt branch 3 times, most recently from 8161be4 to a924def Compare April 6, 2018 14:57
@mamatha-jv
Copy link
Contributor

Hi Mesbah
I am not looking into .java files for now as they are migrated ones.
Please share test execution links if any.

@Mesbah-Alam
Copy link
Contributor Author

@mamatha-jv : I had posted links to some of the test runs in personal builds in the pr: #88

<javac srcdir="${src_dir}"
destdir="${bin_dir}"
fork="true"
executable="${java8_compiler}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not hard code ${java8_compiler}, what about java9, 10, 11?
I think the code works because in top.xml, it happens to set java8_compiler and java9_compiler to java.home if it is not defined. https://github.com/AdoptOpenJDK/openjdk-systemtest/blob/0b730da942494ff5c4aea03593dffd8cf9252ae2/openjdk.build/include/top.xml#L57
For example, the output for jdk10 testing is very confusing:

17:55:24      [echo] java8_home set to /home/jenkins/workspace/openjdk10_j9_systemtest_x86-64_linux/openjdkbinary/j2sdk-image/bin/..
17:55:24      [echo] java8_compiler set to /home/jenkins/workspace/openjdk10_j9_systemtest_x86-64_linux/openjdkbinary/j2sdk-image/bin/javac

https://ci.adoptopenjdk.net/view/System%20tests/job/openjdk10_j9_systemtest_x86-64_linux/6/consoleFull

This is may not be the scope of this PR, but top.xml should be fixed. There is no need to have separate variables for java compiler.

Copy link
Contributor Author

@Mesbah-Alam Mesbah-Alam Apr 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! A new PR has been created for the issue @llxia : #100

@llxia
Copy link
Contributor

llxia commented Apr 17, 2018

I understand this code is migrated. Please create issue about mentioned problem. We can fix it in the future.

@Mesbah-Alam
Copy link
Contributor Author

Related PR : adoptium/openj9-systemtest#19

@Mesbah-Alam Mesbah-Alam deleted the addJlmSystemTestToAdopt branch May 31, 2018 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants