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

[JENKINS-60410] Disable stack trace suppression for tests. #201

Merged
merged 2 commits into from
Feb 26, 2020

Conversation

jeffret-b
Copy link
Contributor

@jeffret-b jeffret-b commented Feb 12, 2020

If this property is set and if the changes in jenkinsci/jenkins#4389 are active, then this changes the default behavior to show full stack traces. (It enables stack traces.) If the property isn't set with 4389 then stack traces are suppressed and some tests that check error results fail. If the property is set without 4389 then it's a no-op.

This matches other existing behavior. See the nearby line for allowing Dispatcher tracing, Dispatcher.TRACE.

It makes testing and troubleshooting a little easier.

This matches other existing behavior. It makes testing and troubleshooting a little easier.
Some tests depend upon test error messages or stack traces.
@jeffret-b
Copy link
Contributor Author

Hmm ... looks like maybe incrementals publishing is broken again.

@jeffret-b
Copy link
Contributor Author

Let's see if incrementals is working now.

@jeffret-b jeffret-b closed this Feb 13, 2020
@jeffret-b jeffret-b reopened this Feb 13, 2020
@timja
Copy link
Member

timja commented Feb 13, 2020

Let's see if incrementals is working now.

I've merged master as it was skipped in the last build due to not being up to date

@jeffret-b
Copy link
Contributor Author

Thanks for that @timja. I forgot about checking if master was up-to-date.

Copy link
Member

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

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

Looks OK, but Dispatcher.TRACE = true; two lines up is a lot nicer.

Hence https://github.com/jenkinsci/jenkins/pull/4389/files#r379862468

Copy link
Contributor

@Wadeck Wadeck left a comment

Choose a reason for hiding this comment

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

✔️ idea
💯 proposal from Daniel for consistency

@jeffret-b
Copy link
Contributor Author

Putting this on-hold for some discussion on the disabling mechanism happening over in the companion core PR. Once I get that addressed, I'll update this one.

@jeffret-b jeffret-b removed the on-hold label Feb 21, 2020
@jeffret-b
Copy link
Contributor Author

Removing the on-hold label as the concerns, especially relating to the other PR in core, have been accepted. This should now be ready for a release so the core PR can proceed.

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.

👍

@oleg-nenashev
Copy link
Member

jenkinsci/jenkins#4389 needs to be merged/released first

@jeffret-b
Copy link
Contributor Author

@oleg-nenashev , since this just adds a property, this one isn't dependent on the change in core. It just doesn't do anything until this release is integrated with that change. This one should be good to go now.

I'm double-checking whether this change in jenkins-test-harness is required for getting the tests to all pass for the PR in core.

@jeffret-b
Copy link
Contributor Author

We will keep this on hold, until I verify the state with the other PR, though.

@jeffret-b
Copy link
Contributor Author

This one is ready to go and the on-hold can be removed. As explained, this is not dependent on the core change. Instead, it enables the tests to pass for the core change.

It would be great to get this merged and released so the core PR can move forward.

@jeffret-b jeffret-b removed the on-hold label Feb 24, 2020
@timja timja merged commit 209214d into jenkinsci:master Feb 26, 2020
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.

7 participants