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

Add functional and systemtest test pipeline for JDK8 into nightly build #2117

Merged
merged 1 commit into from
Jun 14, 2018

Conversation

llxia
Copy link
Contributor

@llxia llxia commented Jun 7, 2018

Issue: #2037

Signed-off-by: lanxia lan_xia@ca.ibm.com

@llxia
Copy link
Contributor Author

llxia commented Jun 7, 2018

Reviewers: @smlambert @jdekonin @AdamBrousseau

@llxia llxia added the comp:test label Jun 7, 2018
case "_sanity":
TEST_JOB_NAME = "Test-Sanity-JDK${SDK_VERSION}-${SPEC}"
if (SPEC.contains("win") || SDK_VERSION.contains("JDK9") || SDK_VERSION.contains("JDK10")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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 pointing out. I will update the changeset.

if (SPEC.contains("win") || SDK_VERSION.contains("JDK9") || SDK_VERSION.contains("JDK10")) {
TARGET_NAMES = ["Sanity"]
} else {
TARGET_NAMES = ["Sanity", "sanity.functional", "sanity.system"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why we are still doing Sanity in this case since it should be a duplicate of sanity.functional?

Copy link
Contributor

Choose a reason for hiding this comment

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

'sanity' target may now == 'sanity.functional' + 'sanity.system' so the 3 are different

Copy link
Contributor Author

@llxia llxia Jun 7, 2018

Choose a reason for hiding this comment

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

I will remove Sanity once sanity.functional is stable. I do not want to cause any testing gap. Sanity should be removed next week.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you've been running your sanity.functional and it works, then I think its safe to switch over right away. Maybe @pshipton has an opinion? It's the same testing just invoked with a different pipeline script right. The change is only for the nightly job right now, the PRs and the OMR Acceptance will continue to use the existing test jobs for now.

if (SPEC.contains("win") || SDK_VERSION.contains("JDK9") || SDK_VERSION.contains("JDK10")) {
TARGET_NAMES = ["Extended"]
} else {
TARGET_NAMES = ["Extended", "extended.functional"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question about duplicate testing

break
case "_extended":
TEST_JOB_NAME = "Test-Extended-JDK${SDK_VERSION}-${SPEC}"
if (SPEC.contains("win") || SDK_VERSION.contains("JDK9") || SDK_VERSION.contains("JDK10")) {
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 about SDK_VERSION

// run tests against the SDK build in the upstream job: Build-JDK${SDK_VERSION}-${SPEC}
jobs["${level}"] = build_with_one_upstream(TEST_JOB_NAME, BUILD_JOB_NAME, jobs["build"].getNumber(), params.VARIABLE_FILE, params.VENDOR_REPO, params.VENDOR_BRANCH, params.VENDOR_CREDENTIALS_ID, params.TEST_NODE)
echo "JOB: ${TEST_JOB_NAME} PASSED in: ${jobs[level].getDurationString()}"
for ( name in TARGET_NAMES ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why this loop is a for and not an each like 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.

Is there a reason why I cannot use for?

Copy link
Contributor

Choose a reason for hiding this comment

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

no, just wondering for consistency.

@@ -33,7 +33,7 @@ def OPENJ9_BRANCH = 'master'
def OMR_REPO = 'https://github.com/eclipse/openj9-omr.git'
def OMR_BRANCH = 'master'

TESTS_TARGETS = '_sanity'
TESTS_TARGETS = '_sanity.tmp'
Copy link
Contributor

Choose a reason for hiding this comment

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

Also want to point out for better awareness. This will allow the OMR build to continue to run the same way it does today. Once the new jobs are proven stable in the nightly, we can remove this tmp target and the corresponding case in the workflow function.

@@ -33,7 +33,7 @@ def OPENJ9_BRANCH = 'master'
def OMR_REPO = 'https://github.com/eclipse/openj9-omr.git'
def OMR_BRANCH = 'master'

TESTS_TARGETS = '_sanity'
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps a question for @pshipton , when we change this target, will it be _sanity (functional and system) or will it be sanity.functional? If it is the latter, I think your default case would need the if win/9/10 also since the OMR build currently does 8 & 10 on all platforms (including win)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the answer of this question should not block this PR. By the time we switch OMR build to use AOJ test pipeline, hopefully win/9/10 issues are fixed. if not, we can add if in default case at that time.

targets.each { target ->
switch (target.trim().toLowerCase()) {
// Due to the following reason, OpenJ9 test pipelines are still used temporarily:
// - waiting for wins machine configuration to be finalized
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably add some issue number(s) here

switch (target.trim().toLowerCase()) {
// Due to the following reason, OpenJ9 test pipelines are still used temporarily:
// - waiting for wins machine configuration to be finalized
// - there is no need to enable JDK9 as it is not supported anymore
Copy link
Contributor

Choose a reason for hiding this comment

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

ref #2020

// Due to the following reason, OpenJ9 test pipelines are still used temporarily:
// - waiting for wins machine configuration to be finalized
// - there is no need to enable JDK9 as it is not supported anymore
// - switch to use AdoptOpenJDK test pipelines for JDK8 and we will switch for JDK10 next step
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an issue blocking us from doing 10 now too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, there is no issue blocking for jdk10. OpenJ9 Jenkins server has been very flaky recently, so I have hard time to run tests. That's why I will enable JDK8 first.

Issue: eclipse-openj9#2037

[ci skip]

Signed-off-by: lanxia <lan_xia@ca.ibm.com>
@llxia
Copy link
Contributor Author

llxia commented Jun 7, 2018

changeset updated for SDK_VERSION and issue links

Copy link
Contributor

@AdamBrousseau AdamBrousseau left a comment

Choose a reason for hiding this comment

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

LGTM other than my 1 question about duplicating sanity functional temporarily.

@smlambert smlambert merged commit cd12bdc into eclipse-openj9:master Jun 14, 2018
AdamBrousseau added a commit to AdamBrousseau/openj9 that referenced this pull request Jun 14, 2018
[skip ci]
Issue eclipse-openj9#2117 eclipse-openj9#2037

Signed-off-by: Adam Brousseau <adam.brousseau88@gmail.com>
AdamBrousseau added a commit to AdamBrousseau/openj9 that referenced this pull request Jun 16, 2018
- Remove old style sanity & extended (functional)
  from nightly builds.
- Machine resources are limited and new style (Adopt Pipelines)
  functional testing is stable, so this removes the
  old style (OpenJ9 pipelines)

Related eclipse-openj9#2117
Issue eclipse-openj9#1450
[skip ci]

Signed-off-by: Adam Brousseau <adam.brousseau88@gmail.com>
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.

3 participants