[JENKINS-36305] Fix TestObject url in order to fix TestActions links #50

Vlatombe · 2016-06-29T09:17:20Z

@reviewbybees

https://issues.jenkins-ci.org/browse/JENKINS-36305

ghost · 2016-06-29T09:19:32Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

jglick · 2016-06-29T17:41:57Z

This seems like it would be incompatible. What was the actual source of the 404? Sounds like a bug in the junit-attachments plugin, no? Here specifically.

Vlatombe · 2016-06-29T17:44:19Z

yes indeed it is incompatible. junit-attachments could implement a workaround by building an absolute path at the line you pointed. But I believe the bug lies in junit-plugin, which is using a taglib expecting a different contract on #getUrl().

hardcoding

Vlatombe · 2016-06-30T12:40:11Z

@jglick do you think it is unacceptable to break compatibility here? With the current implementation, TestActions that want to be exposed in sidebar cannot use relative urls, unlike regular Actions.

jglick · 2016-07-01T12:47:19Z

Perhaps this was not the best design choice, if junit foresaw the possibility of test actions being in the sidebar, but changing this behavior now seems too dangerous when there is a simple solution in the plugin actually affected by the bug.

Vlatombe · 2016-07-07T17:47:09Z

I gave a shot to specifying getUrlName with the following contract in junit-attachments

If the returned string starts with '/', like '/foo', then it's assumed to be
relative to the context path of the Jenkins webapp.

But then I realised this can't work because of https://github.com/jenkinsci/junit-plugin/blob/master/src/main/java/hudson/tasks/test/TestObject.java#L442.

Bottom line: if you don't use relative path, then the action isn't bound so you can't access it.

The current PR is the only way I see to fix it.

oleg-nenashev · 2016-09-24T10:18:35Z

@Vlatombe are you still working on it?

Vlatombe · 2016-09-24T10:33:25Z

@oleg-nenashev I still stand by what I said in my last comment. IMO this PR is the only way to fix properly the link in junit-attachment, and I don't see any simple solution to fix it without affecting junit-plugin.

Downstream of jenkinsci/junit-plugin#50

oleg-nenashev · 2016-09-24T23:14:10Z

After some review 👍 from me

jglick

Not sure I understand the compatibility implications well enough to be comfortable merging. Will leave it to @oleg-nenashev to consider.

jglick · 2017-02-13T14:58:42Z

src/main/java/hudson/tasks/junit/TestObject.java

     *
-     * @return the url relative to {@link TestResult}.


Perhaps we could deprecate the old method and introduce a replacement intended to have different behavior?

I had documented the problem in
https://issues.jenkins-ci.org/browse/JENKINS-36305?focusedCommentId=262717&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-262717
I don't see how it can be fixed otherwise.

oleg-nenashev · 2017-02-16T07:15:18Z

Not sure I understand the compatibility implications well enough to be comfortable merging. Will leave it to @oleg-nenashev to consider.

I should have explained my PoV regarding the compatibility more explicitly. Then I would be able to read it now and restore the context :( Will review from scratch

oleg-nenashev

Apparently I did not. I will leave the decision about merge to the plugin maintainer (if any)

timja

@Vlatombe are you still interested in this PR? or shall we close it.

src/main/java/hudson/tasks/junit/History.java

Vlatombe · 2020-09-30T08:23:18Z

@timja I'll check whether it still makes sense.

Vlatombe · 2020-09-30T09:29:48Z

Updated.

timja · 2020-09-30T11:05:49Z

I’ll take a look next week

timja · 2020-10-05T16:08:56Z

I'm struggling to test this any hints?

I have a freestyle build with this:

rm *.xml
wget https://raw.githubusercontent.com/jenkinsci/junit-plugin/master/src/test/resources/hudson/tasks/junit/pipeline/junit-report-testTrends-first-2.xml

mkdir -p org.twia.vendor.VendorManagerTest
echo 'hello' > org.twia.vendor.VendorManagerTest/attachment.txt

echo "[[ATTACHMENT|${WORKSPACE}/org.twia.vendor.VendorManagerTest/attachment.txt]]"

and I've configured the publisher =/

the attachment isn't showing up

jtnord · 2020-09-30T15:02:25Z

src/main/java/hudson/tasks/test/TestObject.java

    @Override
    public String getUrl() {
-        return '/' + getId();
+        return getRun().getUrl() + getTestResultAction().getUrlName() + "/" + getId();


at first glance this could break https://github.com/jenkinsci/cucumber-testresult-plugin

not really sure how to test this =/, I tried with the tests in https://github.com/jenkinsci/cucumber-testresult-plugin and these: https://github.com/testdouble/java-cucumber-example

jtnord · 2020-10-05T18:01:07Z

I'm struggling to test this any hints?

I have a freestyle build with this:

rm *.xml
wget https://raw.githubusercontent.com/jenkinsci/junit-plugin/master/src/test/resources/hudson/tasks/junit/pipeline/junit-report-testTrends-first-2.xml

mkdir -p org.twia.vendor.VendorManagerTest
echo 'hello' > org.twia.vendor.VendorManagerTest/attachment.txt

echo "[[ATTACHMENT|${WORKSPACE}/org.twia.vendor.VendorManagerTest/attachment.txt]]"

and I've configured the publisher =/

the attachment isn't showing up

normally the "output" needs to be part of the test output not the job console output (and captured by junit (in its sdr-err or std-out element IIRC))

timja

reproduced the bug and verified that this PR fixes it.

I created https://github.com/timja/junit-attachments-test/blob/master/src/test/java/com/github/AppTest.java#L25 to verify it.

Checking @jtnord 's comment before merging

[JENKINS-36305] Fix TestObject url in order to fix TestActions links

5e9935e

Vlatombe added 2 commits June 30, 2016 13:50

[JENKINS-36305] Should retrieve urlName for TestResultAction instead of

40cfa0a

hardcoding

[JENKINS-36305] Fix unit test

04c8855

Remove unused imports

541992f

oleg-nenashev added the needs-review label Sep 24, 2016

Vlatombe added a commit to Vlatombe/junit-attachments-plugin that referenced this pull request Sep 24, 2016

Downstream of jenkinsci/junit-plugin#50

e4d9d9c

Vlatombe added a commit to Vlatombe/junit-attachments-plugin that referenced this pull request Sep 24, 2016

[JENKINS-36305] Fix HTTP404 when browsing test actions from test result

7b60374

Downstream of jenkinsci/junit-plugin#50

Vlatombe mentioned this pull request Sep 24, 2016

[JENKINS-36305] Fix HTTP404 when browsing test actions from test result jenkinsci/junit-attachments-plugin#17

Closed

jglick reviewed Feb 13, 2017

View reviewed changes

oleg-nenashev self-assigned this Feb 16, 2017

oleg-nenashev self-requested a review February 16, 2017 07:13

oleg-nenashev removed their assignment Mar 3, 2018

oleg-nenashev reviewed Mar 3, 2018

View reviewed changes

timja reviewed Sep 22, 2020

View reviewed changes

src/main/java/hudson/tasks/junit/History.java Outdated Show resolved Hide resolved

Merge branch 'master' into JENKINS-36305

1a3a45d

jtnord reviewed Oct 5, 2020

View reviewed changes

timja approved these changes Oct 5, 2020

View reviewed changes

timja merged commit d7d76b4 into jenkinsci:master Oct 8, 2020

Vlatombe deleted the JENKINS-36305 branch October 8, 2020 12:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-36305] Fix TestObject url in order to fix TestActions links #50

[JENKINS-36305] Fix TestObject url in order to fix TestActions links #50

Vlatombe commented Jun 29, 2016

ghost commented Jun 29, 2016

jglick commented Jun 29, 2016

Vlatombe commented Jun 29, 2016

Vlatombe commented Jun 30, 2016

jglick commented Jul 1, 2016

Vlatombe commented Jul 7, 2016 •

edited

Loading

oleg-nenashev commented Sep 24, 2016

Vlatombe commented Sep 24, 2016

oleg-nenashev commented Sep 24, 2016

jglick left a comment

jglick Feb 13, 2017

Vlatombe Sep 30, 2020 •

edited

Loading

oleg-nenashev commented Feb 16, 2017

oleg-nenashev left a comment

timja left a comment

Vlatombe commented Sep 30, 2020

Vlatombe commented Sep 30, 2020

timja commented Sep 30, 2020

timja commented Oct 5, 2020 •

edited

Loading

jtnord Sep 30, 2020 •

edited by timja

Loading

timja Oct 6, 2020

jtnord commented Oct 5, 2020

timja left a comment

[JENKINS-36305] Fix TestObject url in order to fix TestActions links #50

[JENKINS-36305] Fix TestObject url in order to fix TestActions links #50

Conversation

Vlatombe commented Jun 29, 2016

ghost commented Jun 29, 2016

jglick commented Jun 29, 2016

Vlatombe commented Jun 29, 2016

Vlatombe commented Jun 30, 2016

jglick commented Jul 1, 2016

Vlatombe commented Jul 7, 2016 • edited Loading

oleg-nenashev commented Sep 24, 2016

Vlatombe commented Sep 24, 2016

oleg-nenashev commented Sep 24, 2016

jglick left a comment

Choose a reason for hiding this comment

jglick Feb 13, 2017

Choose a reason for hiding this comment

Vlatombe Sep 30, 2020 • edited Loading

Choose a reason for hiding this comment

oleg-nenashev commented Feb 16, 2017

oleg-nenashev left a comment

Choose a reason for hiding this comment

timja left a comment

Choose a reason for hiding this comment

Vlatombe commented Sep 30, 2020

Vlatombe commented Sep 30, 2020

timja commented Sep 30, 2020

timja commented Oct 5, 2020 • edited Loading

jtnord Sep 30, 2020 • edited by timja Loading

Choose a reason for hiding this comment

timja Oct 6, 2020

Choose a reason for hiding this comment

jtnord commented Oct 5, 2020

timja left a comment

Choose a reason for hiding this comment

Vlatombe commented Jul 7, 2016 •

edited

Loading

Vlatombe Sep 30, 2020 •

edited

Loading

timja commented Oct 5, 2020 •

edited

Loading

jtnord Sep 30, 2020 •

edited by timja

Loading