Added interactive navigation to extended tap results#29
Conversation
kinow
left a comment
kinow
left a comment
There was a problem hiding this comment.
Had a very quick look, and found nothing major! I'll run some tests when I find some spare time to work on the plug-in. Or if someone can test and confirm it works for them too, maybe include some screenshots, etc. That would be helpful. 🙂
Thank you for the PR!
-Bruno
|
CI failed but it was during the git clone stage. Kicked Jenkins, waiting for it to complete, but I guess the tests should run fine. |
Hmm, I may elaborate. Still the real action is the best :) Thanx for review! |
|
Yes, tests are passing. I hed messed up a bit with those whitespaces. I will double check tomorrow morning .it is late now here. Thaxn agai for quick review! |
judovana
force-pushed
the
hidePasses
branch
3 times, most recently
from
98de39d to
c822d08
Compare
|
Hello! one more set of show/hide buttons, but otherwise I'm done. If yo do nto have cycles to try it out, will attach screenshots soon. I'm afrtaid the plugin will need some dependecy bumps:( I tried to bump it to be able to build with all jdk8,11 and 17 but failed. Simple jenkins master bump of version did not helped, and when I started to bump also other deps, I ended in testfailures:( |
|
Fresh view: |
|
Hello! Is there any future for this PR? |
|
Hi. Sorry end of the year means lots of deliveries at $work (not using the plugin at $work now, so merely a volunteer). Will review and merge when I find some spare time, sorry. |
|
Sure thing. You had just been supper quick with first reply. So I got confused. No worries, and good luck whatever you are doing! |
kinow
left a comment
kinow
left a comment
There was a problem hiding this comment.
Sorry, had a bit of time to test it today, but mvn clean install failed for me and I didn't have enough time to troubleshoot it. Searching I didn't find a good link to explain how to fix it, so I will have to postpone reviewing this one.
[INFO] -------------------------------------------------------------
[ERROR] COMPILATION ERROR :
[INFO] -------------------------------------------------------------
[ERROR] java.nio.file.NoSuchFileException: /home/kinow/Development/java/jenkins/tap-plugin/target/classes/META-INF/exposed.stapler-beans
at java.base/sun.nio.fs.UnixException.translateToIOException(UnixException.java:92)
at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:111)
at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:116)
at java.base/sun.nio.fs.UnixFileSystemProvider.newByteChannel(UnixFileSystemProvider.java:219)
|
unless I committed and pushed some pom changes which do not show, this is issue with to new jdk. See: The issue we observe on jdk11 and 17 is valid, and is caused by misaligned depndencies in pom. I can elaborate a bit in this direction, but it should be different PR. Note: the project behaves identically with my changes in or out... |
Thanks for investigating!
Yeah, when I was working with Jenkins it was a lot easier to keep track of upcoming breaking changes. But now I work on plug-ins like this one on and off, sometimes with intervals of a year. A lot happens in that period, and normally this "lot" includes new checkstyle or spotbugs changes, new maven enforcer plugin rules, replace HTML tables by divs, move things in the UI, and other things added to the plug-in inherited build configuration that break the build 😥 Your PR looks to be in order, so I apologize as this means it will take a bit longer to be reviewed and merged, at least until I sort out my local build, @judovana . Cheers |
|
Thanx a lot. No problems. I will try to look into the dependency hell. I think I had already seen both 11 and 17 exception(s). It just needs good luck with version numbers :) |
|
Maybe we can merge it and release now? Pretty Please? |
The build is still not working for me and I haven't had time to try to fix it, sorry. |
|
It should be enough if you built by jdk8. Can you? |
Tested with jdk8 and the build is passing now. Once I find spare time (st. eulalia parties ongoing this weekend over here, then st. valentin and carnival… plus $work/life, sorry) will try to review it and if nothing's wrong, merge & release 🎉 |
|
Sure thing :) Happy hacking! |
There was a problem hiding this comment.
Hi @judovana ! Got down with some 🐛 so had to stay home the whole weekend. Good opportunity to catch up with Open Source issues.
It had been ages since I last looked at this code, and seeing some headers with the date of 2010 brings back a lot of memories (but also reminds me how better this code could be, sorry! 🙇 ).
I left a few comments in the code, and had a go at testing the change. The functionality suggested works, but I think we could improve the user experience a little. See screenshots & comments below.
In the screenshot above, it is not immediately clear that the 1 ok, or failed can be clicked. At least we would have to add a cursor: pointer CSS style, I think.
TODO and Bail Out are ignored and cannot be hidden/displayed. Not sure if intentional.
The Nothing when active still displays the TODO element. I assume Bail Out elements will also be displayed.
Might be good to have a help icon explaining each option, especially the "failed details" and "no details". Took me a while to realize what they were supposed to do (as my example was rather simple).
There is no indication to the user of what filters are active or not. IMO instead of clickable text, it would be better to have a toggle element, a check box, etc.
Cheers
Bruno
|
Hello! Thanx a lot for review!
|
|
Hello! Time for som eopensource youa re saying? Here it is - major showstopper of global polution shoudl be fixed. I'm ok with merging, and adding bailedouttodo later once you provide the tap file. How do you see it? Double thanx for review! |
Busy with projects at $work, but this PR is on my GitHub notifications, sorry for the delay! 👍 |
That's unfortunately true!!! (/me hides) 😄 |
|
"and while they discussed, my beard grow long and grey, then I found they have not discussed, they played pokeemons all that long time" |
That was intentional. Where the first set - with numbers, represent direct actions above individual types of results, the second set, behind "show:" represents workflow - where you usualy first wish to enumerate failures, then to see theirs details, and then again hide the traces, but keep them in listing mode, so you can investigate on demand. The "show:" donot work as toggles, they are setting up the view. To reset it, you can use nothing/all or another view. The views are still affected by the "togles" (first set with numbers). The "toggles" first set, may be combined. The "views", second set, always reset to defined state. I would liek to keep this functionality, as that is why I designed it. However I will redo the GUI. eg: with
in mind wdyt? |
judovana
force-pushed
the
hidePasses
branch
3 times, most recently
from
4896960 to
6654e6a
Compare
done
explained in #29 (comment)
Fixed. The links are now toogle buttons with on/of and mixed state. The mixed state is necessary, as you can click to "show all" and then user hide some manually. In that moment, the button move from on/of to middle mixed state.
I had fixed like this: once you click some filter, the buttons realign themselves, so they are properly on/off (no middle state can happen here) as filter set it up
For features, jiras are optional. Do yuou really insists for creating "jira just for jira", when there is all info and history and conversation in this PR?
If you import the selenium suite, I will be glad to add tests. But, please, do not let it block this pr.
From what I may say, your tap file do not represent the feature. In tap files - at least as tap plugin is visualizing it, are two types of messages - the "title", which is the one you see in your tap file and view. That is followed by directive. As that is part of the title row and have directive, I had intentionally left it in.
Correct. As I personally never met bailed out tests, I was unsure how to proceed with them. I understand them as special - critical - case of failure, thus they are shown always, unless you specifically hide "bailed out tests" (it was link before, it is button now)
As discussed, this is no show stopper. I'm aware about pure css show/hide https://dev-bay.com/css-show-hide-div-without-javascript/ , and I actually like to use it:
Yes. That is the +/- "button". It have the new shiny shape, better tooltip, and remains toggled, if used. I hope you will like it :)
Right. That was error. Fixed now. Yet again, it was specific to yours tap file, not the "big real one" I'm attaching. But I'm glad it was hit, and thus fixed.
Simialrly as with bailed. failed/bailed and todo/skipped have each theirs own button. Is it ok?
As written a bit earlier, I will be doing individual commits, and the PR will be squeezed/squashed during merge. Ibelive it is good practice, and it is making life easier for both reviwere and contribnutor.
Where this is heavily necessary, please postpond it until this PR is in. To keep re-basing bigger changeset over moreover robotic reformatings, refactroings and re-spotbugging is energy draining. TYVM! The ball is in your hads now! |
|
motivation screenshot: https://snipboard.io/4RTEs7.jpg |
|
hello! its december! is still end of the year valid deadline?-) |
|
ping please? |
- possibility to hide/show - passed/failed/ignored tests - diagnosticd details - individual diagnostic items - number is now direct url to itself tested in firefox and chromium The todo and bail out items ar enot covered, as I do nto have exmaple tap file with them
They can be shown/hide separately, but otherwise they follow ignored tests
in mass fields, coments follows passes and bailed out failures
The buttons to show/hide content are now on separate line the toogle state represent elements state (all viisble, all invisible and mixed content) Buttons are shown also for non direct choices liek detaisl. Old bug with space in class enforced to be fixed (all spaces now repalced by _) Old bug with NPEs on element by id or in missing style fixed
as fixButtons was called aprox 15x in set-view shortcut methods, there was slight (up to 0.5s) delay glitch after click. Added extendedTapsetInteractives-wide local variable wich can disable repeated fixButtons calls
|
@kinow 2024! Happy new year! ping please? |
|
@kinow hello! First of all please s/they/he... Its jsut me, none else, and I'm quite normal "he".
It is still the same org - jenkinsci - I had requested first time after 9 months of inactivity in project, with good intention to take over plugin I depend on where maintainer is gone. As part of the issue you were contacted, and we agreed on somehow finishing it. Yet you disappeared again.@MarkEWaite suggested to wait a two weeks, and then grant me the permissions to keep plugin viable. I had waited 3 more months before becoming unpleasant again, and bothering everybody with my tinny PR again. Now, be sure that I do not want to maintain voluntarily any more legacy open-source projects. But everything is better then spend rest of my life in fork. The goal of opensource projects gathering under various foundations is, that they do not die. If maintainer leave, the organization can grant another willing soul a maintainer ship. Obviously jenkins government board is not exactly united in this.
Not only that. The plugin was terribly outdated and literally unsecure. As I already said, I inherited the team who needed that - on public instance - so it was obvious it have to be acted. First things first - the js/html navigation. It was no pleasure to do, but the plugin as is, is useless on big test bases with. I deal with tesbases of 100000 tests and more. The usability of tap plugin ends somewhere around 100(see bottom) Luckily for us all, in October and December you actually worked on that, so at least something positive come out.
I had replied - fixed all you wanted - 8.11.2023 That is quite long before christmas global outage.
@kinow, that is perfectly ok. If you would just drop a note "I saw that, it will need tuning", I would wait much more peacefully.
Maybe I would, maybe not - but do you think there is any motivation left, when we communicate on time base like Voyger->Earth->Voyger ...Oh wait.. that is much faster :)
If I ever said something what made you (or anybody of other from jenkins community) then I'm sorry and thanx for your acceptance of cultural mishmash.
Thats perfectly ok, but you must accept your responsibility and become reliable maintainer again. As I wrote above - I really do not want to inherit more legacy stuff. But everything is better then fork. Every member of the community have right to contribute. If overarching organization can not fill that gap of unresponsive owner, then forks starts appearing, fork wars starts, and there is no benefit nowhere until one of the forks gain wide acceptacne over original which then dies forgotten. In meantime many time, resources, bugfixes and features are lost.
I'm looking forward for your feedback and ideas (really, a lot I do). But please, have some mercy and do not leave my waiting in queue like the most minor citizen, while you are doing other stuff in the same codebase. Or beeing silent for months and months without saying a word.
I have to highlight - that is not just because of the PR, but also due to the terrible outdated codebase. You have worked on that in past, so thats much better. But yet again - you are forcing me to keep using fork in production, without any reasons. The PR is fixing severe issue in usability.
That is a year old CVE, and usually only https://github.com/jenkinsci/nested-view-plugin/blob/27c08752b32e6a7fa1616db05653a67bc91f462b/src/main/resources/index.jelly#L1C1 so I'm not sure why you are taking that as deffender.
Can you be little bit more specific (the xss is private bug). I had already offer you a help (and by PR#30 I started to work on that), but no one sane want to have several PRs to half-dead project (last commit 5 years ago before I stepped in, 4 rotten PRs...), opened. |
|
Thanx for merging the fix to the CVE! I will rebase this one to apply on your changes. |
|
Rebased in #38 all comments and screenshots remain valid in time of creation of #38 |
2 participants
tested in firefox and chromium
The todo and bail out items ar enot covered, as I do nto have exmaple tap file with them