Allow for moving truncation into the implementing API #68

mrginglymus · 2021-01-01T13:21:31Z

As usual, not claiming this is the right way to do it, but I think it is a way to do it. We can then update the github-checks-plugin to call getSummary(65536, " see [Jenkins](url)") when creating its Output. It does mean that consumers can't set the 'see more' text, but it means that the GHChecks plugin can confidently use markdown.

Of course one of the other options is to add something to the GitHub Checks plugin that can intelligently truncate markdown. But I've no idea what sort of libraries are already out there to help with that. That would also prevent the user from being able to control chunk size.

timja · 2021-01-01T13:50:29Z

'see more' text, but it means that the GHChecks plugin can confidently use markdown.

not ideal as in the junit plugin we link to the test report

intelligently truncate markdown.

would be surprised if we could get something that would work without it being overly complicated

mrginglymus · 2021-01-01T14:42:49Z

not ideal as in the junit plugin we link to the test report

That does mean the junit plugin assumes that the checks api implementation supports markdown. That seems likely to be the case, but who knows.

We could make things considerably more structured - have something like a ChecksTruncatedText class that accepts a url and a list of strings, and then is invoked by the implementation that turns that constructs the read more link into the appropriate markdown and then does the chunking based on its own knowledge of maximum lengths - but that would be even bigger changes to the API.

XiongKezhi · 2021-01-01T14:54:54Z

I don't think we should validate the message here because the overflow should be a problem that the implementations should take care of IMO. Since when they are implementing this API according to a specific platform, they should refer to the official documents (e.g. Github documents for checks API) to do the validations. Maybe in the status publisher, it is ok to check the max size because it is a consumer that tries to make every work, but adding too much validation here makes the API complicated IMO.

mrginglymus · 2021-01-01T15:02:53Z

I think thats maybe why I'd like to see some kind of TruncatedStringBuilder exposed from the checks publisher so we end up with something like:

new ChecksOutput.Builder()
  .withSummary(publisher.getTruncatedStringBuilder()
    .withReadMoreUrl(url)
    .addText(...)
    .addText(...)
    .build())

Then its up to the implementers to provide an implmentation of TruncatedStringBuilder that sets the correct limits, and converts the 'read more' url into the appropriate formatting.

(for clarity, what I've written there is not what I think the latest commit does!)

XiongKezhi · 2021-01-01T15:06:58Z

Would the read more URL different from the details URL?

XiongKezhi · 2021-01-01T15:17:11Z

src/main/java/io/jenkins/plugins/checks/api/ChecksOutput.java

+            return withText(new TruncatedString.Builder().addText(text).build());
+        }
+
+        public ChecksOutputBuilder withText(final TruncatedString text) {


adding the truncated string builder in line 66 make sense since it makes things convenient for implementations, but since we already have details URL, I'm not sure the see more link would be necessary for consumers, if not, the truncated string build is unnecessary as well IMO.

#68 (comment)

the see more link was just a helpful extra added in junit it could probably be skipped but truncating is needed afaict

mrginglymus · 2021-01-01T15:33:32Z

Would the read more URL different from the details URL?

Good point. I'm not sure it would be. But possibly difficult to get to.

timja · 2021-01-01T15:54:25Z

I don't think we should validate the message here because the overflow should be a problem that the implementations should take care of IMO. Since when they are implementing this API according to a specific platform, they should refer to the official documents (e.g. Github documents for checks API) to do the validations. Maybe in the status publisher, it is ok to check the max size because it is a consumer that tries to make every work, but adding too much validation here makes the API complicated IMO.

the problem with implementations doing it is you would get broken markdown and truncating at a sane point would be difficult

Would the read more URL different from the details URL?

I don't think so

mrginglymus · 2021-01-01T16:00:25Z

Actually the worry about read more being in markdown is pointless as consumers are already assuming markdown as the format for long text fields. I guess if other implementations don’t support markdown they can handle that themselves.

mrginglymus · 2021-01-01T20:46:03Z

So, condensing the above I think we have:

Consumers should not have to worry about truncating long text fields per-platform; this should be handled by the implementer.
We should not worry about the format of the truncation text, as the consumer is already providing the body text in whatever format they like. Perhaps we should document in the api that the implementers are expected to be able to handle markdown; if their platform does not then they may wish to convert/simplify it as appropriate.
Consumers should be able to specify their own truncation text.

Does that sound reasonable? I will work out some code roughly to that effect.

codecov · 2021-01-01T21:36:04Z

Codecov Report

Merging #68 (e92aa81) into master (c427313) will increase coverage by 0.66%.
The diff coverage is 91.54%.

@@             Coverage Diff              @@
##             master      #68      +/-   ##
============================================
+ Coverage     86.51%   87.18%   +0.66%     
- Complexity       78       86       +8     
============================================
  Files            14       15       +1     
  Lines           408      476      +68     
  Branches         17       24       +7     
============================================
+ Hits            353      415      +62     
- Misses           49       54       +5     
- Partials          6        7       +1

Impacted Files	Coverage Δ	Complexity Δ
...va/io/jenkins/plugins/checks/api/ChecksOutput.java	`95.34% <77.77%> (-4.66%)`	`8.00 <3.00> (ø)`
...io/jenkins/plugins/checks/api/TruncatedString.java	`93.54% <93.54%> (ø)`	`8.00 <8.00> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c427313...e92aa81. Read the comment docs.

timja

looks good to me (although light on tests), I assume another PR coming to github checks to implement the end to end?

mrginglymus · 2021-01-01T21:44:04Z

Yep. I'll pull over (and extend) the tests from #66

timja · 2021-01-02T09:22:09Z

src/main/java/io/jenkins/plugins/checks/api/TruncatedString.java

+     */
+    @CheckForNull
+    public String build() {
+        return chunks.isEmpty() ? null : String.join("", chunks);


what do you think of the statement:

the end of the build log is more interesting than the beginning, there should be more fine grained control over the truncation?

This is more relevant to #66

or should we just say YAGNI and improve it later if required?

Sounds reasonable, and certainly easy enough to implement.

I also think there’s cause to say “allow newine truncation”, so that if you know you’re just putting plain text in (e.g. a large build log) as a single chunk, you can do a new line truncation. Combine that with reverse truncation and you have everything I think.

mrginglymus · 2021-01-09T14:35:42Z

Ok this may now be hugely over-engineered, but I think it works. The tests hopefully give an idea of what it can do.

timja · 2021-01-09T16:27:15Z

Ok this may now be hugely over-engineered, but I think it works. The tests hopefully give an idea of what it can do.

seems sane any reason it's in draft?

mrginglymus · 2021-01-09T16:36:39Z

I’ll rebase #66 on top of it to validate it first, if a rebase is ok - otherwise I’ll spin a new branch to test it.

I’ll also get a PR open for github checks.

mrginglymus · 2021-01-09T19:27:25Z

I'm glad I tried using it on #66- definitely have some changes to make.

mrginglymus · 2021-01-09T20:50:06Z

src/test/java/io/jenkins/plugins/checks/ArchitectureTest.java

-
-    @ArchTest
-    static final ArchRule NO_PUBLIC_TEST_CLASSES = PluginArchitectureRules.NO_PUBLIC_TEST_CLASSES;
+//    Todo: this seems to prevent paramteized non-integration tests.


So I know that I'm using a Junit4 style test here, but that's because the parametrized options for Junit5 seem to require a huge amount of repetitive boilerplate for this kind of use-case.

It seems easy enough to fix this rule upstream to permit Junit4 style tests:

/** Junit 5 test classes should not be public. */ public static final ArchRule NO_PUBLIC_TEST_CLASSES = noClasses().that().haveSimpleNameEndingWith("Test") .and().haveSimpleNameNotContaining("_jmh") .and().doNotHaveModifier(JavaModifier.ABSTRACT) + .and().areNotAnnotatedWith(RunWith.class) .and().haveSimpleNameNotEndingWith("ITest") .should().bePublic();

if that's deemed acceptable?

@uhafner I'd appreciate your input on this, as you're down as the author of these rules.

I'm fine with using JUnit 4 in this module. It would be simpler if you would just inline the rule here and adapt it accordingly (or remove it completely).

src/main/java/io/jenkins/plugins/checks/api/TruncatedString.java

XiongKezhi · 2021-01-10T07:33:21Z

src/main/java/io/jenkins/plugins/checks/api/TruncatedString.java

+    public String build(final int maxSize) {
+        List<String> parts = getChunks();
+        if (truncateStart) {
+            Collections.reverse(parts);


The logic here is a little bit hard to understand at a start (I even left a comment first asking why we want a reversed output), since it requires you to understand the implementation of the joiner first and the join method in reverse acknowledges this special case. So, although the joiner is just an embedded class, adding another field (e.g. reverse) and set it identical to truncateStart makes it easier to understand IMO.

Yes, I agree that it's a bit manky. I think part of the problem is that we reverse it in two different places. I think I should be able to refactor it so that the Joiner just retuns a plain list of strings and we do the reversal at the end - so the joiner doesn't need to know about reverse; although I'm not sure if that can be done without also moving the truncation text to the start. Would that make more sense? I think it might... so should

1. Some 2. Very 3. Long 4. Output 5. Here

Truncated to three lines would come out in reverse come out as

4. Output 5. Here Output truncated!

or

Output truncated! 4. Output 5. Here

I'm thinking possibly the latter.

src/main/java/io/jenkins/plugins/checks/api/ChecksOutput.java

…ner from TruncatedString

mrginglymus marked this pull request as draft January 1, 2021 13:22

XiongKezhi reviewed Jan 1, 2021

View reviewed changes

mrginglymus force-pushed the limits branch from 1d9b03d to 81f8c65 Compare January 1, 2021 21:21

Allow for moving truncation into the implementing API

46ff97c

mrginglymus force-pushed the limits branch from 81f8c65 to 46ff97c Compare January 1, 2021 21:28

timja reviewed Jan 1, 2021

View reviewed changes

timja reviewed Jan 2, 2021

View reviewed changes

More options for truncation

034e114

mrginglymus marked this pull request as ready for review January 9, 2021 16:34

Merge branch 'master' into limits

32263c4

Consolidate truncated strings into single class

5fa067d

mrginglymus commented Jan 9, 2021

View reviewed changes

mrginglymus requested review from timja and XiongKezhi January 9, 2021 20:55

XiongKezhi reviewed Jan 10, 2021

View reviewed changes

XiongKezhi added the enhancement New feature or request label Jan 10, 2021

mrginglymus added 2 commits January 10, 2021 11:43

Review feedback

a320167

Move trunction text to start when truncating from start, decouple Joi…

e92aa81

…ner from TruncatedString

This was referenced Jan 10, 2021

Truncate long text fields jenkinsci/github-checks-plugin#101

Merged

Use truncated strings from checks api jenkinsci/junit-plugin#236

Open

mrginglymus requested a review from XiongKezhi January 10, 2021 12:41

XiongKezhi approved these changes Jan 11, 2021

View reviewed changes

XiongKezhi merged commit c83076d into jenkinsci:master Jan 12, 2021

timja mentioned this pull request Feb 8, 2021

Output pipeline summary #66

Merged

v1v mentioned this pull request Feb 19, 2021

Truncate long text fields for the githubCheck elastic/apm-pipeline-library#997

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow for moving truncation into the implementing API #68

Allow for moving truncation into the implementing API #68

mrginglymus commented Jan 1, 2021 •

edited

Loading

timja commented Jan 1, 2021

mrginglymus commented Jan 1, 2021

XiongKezhi commented Jan 1, 2021

mrginglymus commented Jan 1, 2021 •

edited

Loading

XiongKezhi commented Jan 1, 2021

XiongKezhi Jan 1, 2021

timja Jan 1, 2021

mrginglymus commented Jan 1, 2021

timja commented Jan 1, 2021

mrginglymus commented Jan 1, 2021

mrginglymus commented Jan 1, 2021 •

edited

Loading

codecov bot commented Jan 1, 2021 •

edited

Loading

timja left a comment •

edited

Loading

mrginglymus commented Jan 1, 2021

timja Jan 2, 2021

mrginglymus Jan 2, 2021

mrginglymus commented Jan 9, 2021

timja commented Jan 9, 2021

mrginglymus commented Jan 9, 2021

mrginglymus commented Jan 9, 2021

mrginglymus Jan 9, 2021

mrginglymus Jan 9, 2021

uhafner Jan 9, 2021

XiongKezhi Jan 10, 2021

mrginglymus Jan 10, 2021

Allow for moving truncation into the implementing API #68

Allow for moving truncation into the implementing API #68

Conversation

mrginglymus commented Jan 1, 2021 • edited Loading

timja commented Jan 1, 2021

mrginglymus commented Jan 1, 2021

XiongKezhi commented Jan 1, 2021

mrginglymus commented Jan 1, 2021 • edited Loading

XiongKezhi commented Jan 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrginglymus commented Jan 1, 2021

timja commented Jan 1, 2021

mrginglymus commented Jan 1, 2021

mrginglymus commented Jan 1, 2021 • edited Loading

codecov bot commented Jan 1, 2021 • edited Loading

Codecov Report

timja left a comment • edited Loading

Choose a reason for hiding this comment

mrginglymus commented Jan 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrginglymus commented Jan 9, 2021

timja commented Jan 9, 2021

mrginglymus commented Jan 9, 2021

mrginglymus commented Jan 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrginglymus commented Jan 1, 2021 •

edited

Loading

mrginglymus commented Jan 1, 2021 •

edited

Loading

mrginglymus commented Jan 1, 2021 •

edited

Loading

codecov bot commented Jan 1, 2021 •

edited

Loading

timja left a comment •

edited

Loading