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.

Sign up for GitHub

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

Jump to bottom

Add Integration Tests + fixes for JENKINS-56312 and JENKINS-57320 #140

Merged
merged 8 commits into from
May 3, 2019
Merged

Add Integration Tests + fixes for JENKINS-56312 and JENKINS-57320 #140

merged 8 commits into from
May 3, 2019

Conversation

batmat
Copy link
Member

@batmat batmat commented Apr 19, 2019

Supersedes #133

@batmat batmat mentioned this pull request Apr 19, 2019
Closed
batmat
batmat commented Apr 19, 2019
View reviewed changes
Jenkinsfile
parallel(branches)

// Integration testing, using a locally built Docker image
def itBranches = [:]
Copy link
Member Author

Choose a reason for hiding this comment

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

There is definitely some level of copy paste below. I think we'll want to factor this out at some point.

But I didn't want to spend too much making this generic yet (like having a way to simply provide the command the pass, the expected result, etc.). I'd rather first have this merged as-is, once green, and we iterate on growing the test coverage and will refactor in followups.

@batmat
Copy link
Member Author

batmat commented Apr 19, 2019

The current ITs demonstrate AFAICT something does not work.
When I reverted #115 in #133, ITs would get back to green.

No time to dig into it now, but hopefully this will help us fix the situation now we can reproduce and track the expected behavior for PCT.

MRamonLeon
MRamonLeon approved these changes May 3, 2019
View reviewed changes
Copy link

@MRamonLeon MRamonLeon left a comment

Choose a reason for hiding this comment

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

Fine to merge it after running fine when the failure found is resolved.

@oleg-nenashev
Copy link
Member

Finally, the regression root cause is in #116 which was then improperly fixed in #131 . The latter fix actually prevents PCT from running properly on any community-provided jenkins.war

@oleg-nenashev oleg-nenashev changed the title [WIP] Add Integration Tests [WIP] Add Integration Tests + fixes for JENKINS-56312 and JENKINS-57320 May 3, 2019
@MRamonLeon
Copy link

Cool!!!!

Screenshot from 2019-05-03 15-53-01

oleg-nenashev
oleg-nenashev reviewed May 3, 2019
View reviewed changes
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

I think it is OK to merge as is, but it would be great to fix copy-paste

Jenkinsfile
sh 'make docker'
}

stage('Download Jenkins 2.164.1') {
Copy link
Member

Choose a reason for hiding this comment

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

Use 2.164.2?

Jenkinsfile
}
}

itBranches['buildtriggerbadge:2.10 tests success on JDK8'] = {
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to prevent copy-paste ?

oleg-nenashev
oleg-nenashev approved these changes May 3, 2019
View reviewed changes
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

approving it actually

alecharp
alecharp approved these changes May 3, 2019
View reviewed changes
Copy link
Member

@alecharp alecharp left a comment

Choose a reason for hiding this comment

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

👏 🙇‍♂️

@oleg-nenashev
Copy link
Member

We will fix the copy-paste later. Since PCT is broken, I will just merge it

@oleg-nenashev oleg-nenashev merged commit 9842626 into jenkinsci:master May 3, 2019
@oleg-nenashev oleg-nenashev changed the title [WIP] Add Integration Tests + fixes for JENKINS-56312 and JENKINS-57320 Add Integration Tests + fixes for JENKINS-56312 and JENKINS-57320 May 3, 2019
@batmat batmat deleted the add-ITs-take-2 branch May 3, 2019 14:28
@oleg-nenashev oleg-nenashev mentioned this pull request Jul 9, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alecharp alecharp alecharp approved these changes

@oleg-nenashev oleg-nenashev oleg-nenashev approved these changes

@MRamonLeon MRamonLeon MRamonLeon approved these changes

@raul-arabaolaza raul-arabaolaza Awaiting requested review from raul-arabaolaza

Assignees
No one assigned
Labels
internal
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@batmat @oleg-nenashev @MRamonLeon @alecharp