replaceDescription is not robust

[basil]

Problem

This is a fix for an unreleased regression. As of #8278, hudson.tasks.junit.JUnitResultArchiverTest#setDescription started failing in PCT with:

AssertionError: no form found
	at org.junit.Assert.fail(Assert.java:89)
	at hudson.tasks.junit.JUnitResultArchiverTest.findForm(JUnitResultArchiverTest.java:228)
	at hudson.tasks.junit.JUnitResultArchiverTest.testSetDescription(JUnitResultArchiverTest.java:216)
	at hudson.tasks.junit.JUnitResultArchiverTest.setDescription(JUnitResultArchiverTest.java:196)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
	at org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:606)
	at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:299)
	at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:293)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.lang.Thread.run(Thread.java:833)

Evaluation

After debugging this, I can see that #8278 exposed a pre-existing bug by making the browser's behavior more strict.

The original bug was actually introduced in #6511. Here we have a Jelly editable description in the JUnit plugin that sets a permission attribute (required), a description attribute (optional), but no submission URL (also optional). This seems like a completely valid thing to do, but the code in editable-description.js from #6511 only handles two cases:

1. Description and submission URL are both set,
2. Neither are set.

Here the description is set, but the submission URL is not set, and the code in editable-description.js from #6511 does not handle this third case. It invokes replaceDescription with a non-null description (the one from the JUnit plugin's getDescription() method) and a null submission URL. The replaceDescription function, similarly assuming that either both are set or neither are set, sees that the submission URL is not undefined (that's right, it's defined as null) and passes this into the parameters object.

So far the code is buggy, but this was never a problem in practice in the Prototype implementation of replaceDescription (because Prototype converted null values into the empty string) or before #8278, but it is a problem after #8278. If I had to guess why #8278 made any difference, it would be that adding the header possibly puts the browser in a more strict mode. In any case the root cause of this whole problem is not #8278 but rather that #6511 failed to handle this legitimate case.

Solution

To fix this I read the documentation noting that

All modern web browsers return null when the specified attribute does not exist on the specified element.

so I fixed the (sole) caller of replaceDescription to always call it with the result of Element#getAttribute (either a valid string or null) and then set the description and parameters separately in the form object in replaceDescription based on whether or not those passed-in values were null and non-empty. There is no need to check for undefined anymore since that is never returned by Element#getAttribute.

Testing done

hudson.tasks.junit.JUnitResultArchiverTest#setDescription failed after #8278 (not yet released) but without this PR and now passes with this PR.

Proposed changelog entries

N/A

Proposed upgrade guidelines

N/A

Submitter checklist

Beta Give feedback

1. The Jira issue, if it exists, is well-described.
2. The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples). Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
3. There is automated testing or an explanation as to why this change has no tests.
4. New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
5. New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
6. New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
7. For dependency updates, there are links to external changelogs and, if possible, full differentials.
8. For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

Beta Give feedback

1. There are at least two (2) approvals for the pull request and no outstanding requests for change.
2. Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
3. Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
4. Proper changelog labels are set so that the changelog can be generated automatically.
5. If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
6. If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

[basil]

This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process. Thanks!