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-58102] GlobalAnnotator does not annotate lines that are part of a multi-line color sequence #28

Merged
merged 4 commits into from Jul 5, 2019

Conversation

@basil
Copy link
Contributor

basil commented Jun 23, 2019

See JENKINS-58102. Fixes a bug introduced in #25 that is only observable when timestamper is used with Pipeline and the multi-line escape sequence support introduced in jenkinsci/ansicolor-plugin#137. The problem is that GlobalAnnotator doesn't account for the <span> elements introduced by ansicolor when looking for timestamps to annotate. The fix is similar in approach to the existing code that accounts for the <span> elements introduced by Pipeline.

I wrote a new test that exercises this use case. When the test is executed against Jenkins 2.145 and ansicolor 0.6.2 (with the src/main changes from this PR reverted), it fails with the following stack trace:

15:23:35 [2019-06-23T22:23:35.328Z] Finished: SUCCESS expected:<21> but was:<17>
	at org.junit.Assert.fail(Assert.java:88)
	at org.junit.Assert.failNotEquals(Assert.java:834)
	at org.junit.Assert.assertEquals(Assert.java:645)
	at hudson.plugins.timestamper.pipeline.PipelineTest.globalDecoratorAnnotator(PipelineTest.java:101)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
	at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
	at org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:552)
	at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:298)
	at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:292)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.lang.Thread.run(Thread.java:748)

When the test is executed against Jenkins 2.145 and ansicolor 0.6.2 (with the src/main changes in this PR), the test passes.

Unfortunately, I couldn't use ansicolor 0.6.2 in this PR because it requires Jenkins 2.145 or later, and only 73% of timestamper users are on Jenkins 2.145 or later. Rather than drop support for 27% of the user base for the sake of one test, I compromised by using ansicolor 0.5.3, whose minimum Jenkins version is 2.121.2. When run against ansicolor 0.5.3, the new test isn't exercising the changes in src/main from this PR. However, it is exercising the bulk of the new logic introduced in #25, which is still a net improvement from the state of affairs before this PR.

…f a multi-line color sequence
@jglick jglick requested review from jglick and dwnusbaum Jul 1, 2019
@jglick

This comment has been minimized.

Copy link
Member

jglick commented Jul 5, 2019

only 73% of timestamper users are on Jenkins 2.145 or later

But virtually all of those who have already updated to 1.9 are. Just bump the jenkins.version, say to 2.150.3, and be done with it.

@basil

This comment has been minimized.

Copy link
Contributor Author

basil commented Jul 5, 2019

Just bump the jenkins.version, say to 2.150.3, and be done with it.

Done.

@jglick
jglick approved these changes Jul 5, 2019
Copy link
Member

jglick left a comment

A tested fix, good. The broader issue is that GlobalAnnotator should not have to be doing these hacks to begin with, which suggests a logic error somewhere in console handling.

@jglick
jglick approved these changes Jul 5, 2019
@jglick jglick merged commit 62237da into jenkinsci:master Jul 5, 2019
2 checks passed
2 checks passed
continuous-integration/jenkins/incrementals Deployed to Incrementals.
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
@jglick jglick changed the title JENKINS-58102 GlobalAnnotator does not annotate lines that are part of a multi-line color sequence [JENKINS-58102] GlobalAnnotator does not annotate lines that are part of a multi-line color sequence Jul 5, 2019
@jglick jglick added the bug label Jul 5, 2019
@basil basil deleted the basil:JENKINS-58102 branch Jul 6, 2019
@basil basil mentioned this pull request Jul 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.