-
-
Notifications
You must be signed in to change notification settings - Fork 552
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
XWIKI-21914: Inconsistency of icons for "delete" and "close" actions #3107
Conversation
* Changes cross icons to trash icons
* Changes cross icons to trash icons
@tkrieck the changes look good but it actually might impact some integration tests: we do use some UI elements for those. Might be great if you could run integration tests on |
Ok, but I am not familiar with these tests, if it's not much trouble, can you point me to some documentation on them? |
https://dev.xwiki.org/xwiki/bin/view/Community/Testing/DockerTesting/ It's a bit much to get through at first. Here is how I'd have tested it.
The tests should take a while, they will start their own instance of XWiki. If the build passes (everything is green at the end, opposite to failing where you see a lot of red), you're all good! The tricky part later on is to find what tests to pass (ideally we'd check all of them, but it takes a long computation time and we can't do that, so you need to find the ones most relevant to your changes). |
Using: Gave me this error, however, it is also happening in the master branch. So I think it is not because of my changes |
Hi @tkrieck ! I could not get the error on the master branch locally, and I couldn't see it in CI (see https://ci.xwiki.org/job/XWiki%20Environment%20Tests/job/xwiki-platform/job/master/558/ ). You need to rebuild every module where you made changes if you want to test master. Somehow it seems one of this module is failing the tests. Failing test: from the log you can see that the confirm alert is not found where it should be. My guess is that it's here, but the updated DOM makes it so that it doesn't match the selector we used in the tests anymore. You probably need to find this selector and update it so that the tests do not fail anymore. |
This test is currently known to fail on master when firefox is used for the docker tests, see GE only the builds tagged with chrome are passing. If you want to see everything passing successfully locally, you can run But if it is the only failing test, I would consider that your changes are not breaking any test. |
Thanks! I'll try that |
I've managed to run the tests after dealing with some environment issues. It passes with: However, it fails when I use the command referenced by @manuelleduc on
The failure message is: I am unsure on how to proceed from here. |
That's expected I think. You can get rid of that error by removing the |
Jira URL
https://jira.xwiki.org/browse/XWIKI-21914
Changes
Description
"cross"
icons to"trash"
Clarifications
Screenshots & Video
Executed Tests
Expected merging strategy
*