-
Notifications
You must be signed in to change notification settings - Fork 270
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
Libreoffice mail use local servers #3320
Conversation
products/sle/main.pm
Outdated
@@ -380,7 +380,10 @@ sub load_x11regression_documentation { | |||
loadtest "x11regressions/libreoffice/libreoffice_mainmenu_components"; | |||
loadtest "x11regressions/libreoffice/libreoffice_recent_documents"; | |||
loadtest "x11regressions/libreoffice/libreoffice_default_theme"; | |||
loadtest "x11regressions/libreoffice/libreoffice_pyuno_bridge"; | |||
loadtest "x11regressions/evolution/evolution_prepare_servers"; | |||
if (get_var("VERSION") =~ /12/) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't you want to run this other versions? Better use sle_version_at_least
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reason is the same as for evolution case, we want to have an exception in case of SLE > 12, which is not supported yet (like 15). sle_version_at_least would match minimal requirement, not the top limit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't understand. At first you can also turn around by saying ! sle_version_at_least
or implement sle_version_at_most
easily. Also, if we assume that in general the same module should also be supported on sle15, simply make it sle_version_at_least('15')
and we will need to take care as soon as we have a sle15 test that fails accordingly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
evolution_prepare_servers will fail in case of >12 with unsupported system information.
I just want skip libreoffice_pyuno_bridge in this case, in same way as evolution tests are skipped to avoid failure following tests (and avoid confusion of reviewing person).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@okurz the test installs from sle12 repos in ibs. Not a module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am afraid with something like this we would silently loose test coverage and not realise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@okurz Will it be fine, If I remove that condition? You are probably right and I can partly agree with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, IMHO it's better to unconditionally trigger this. When it will fail on SLE 15 then we will need to think about how to handle it there. Maybe we will just use a version specific repository when we get to that point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@okurz It is fixed now, also for the other test which used same logic (which is not necessary).
Condition would silently skip tests in case of SLE other than 12, it will be better to have failure more obvious.
Fix poo#20570: Test libreoffice_pyuno_bridge depends on infrastructure located in APAC region. This change allows usage of local servers.
LGTM |
would this allow to also run the tests on openSUSE? I assume they are not triggered there yet |
These tests are not run on openSUSE. If there is a need for it, they will have to be updated. |
Sure, I got this. So you confirmed that I understand it correctly, in before it could not have worked because the tests would rely on internal servers. Still, that does not make them miraceously working |
@okurz If there is need to have it compatible with openSUSE, I'm open to make it. It just wasn't in the original scope. If you agree, I will open new poo for myself related to openSUSE. And what about merging this fix for libreoffice? |
of course, great work! I expected someone else to also provide some feedback but at least no one is strongly against this ;-) |
@okurz I just created poo#20696 |
Fix poo#20570: Test libreoffice_pyuno_bridge depends on infrastructure
located in APAC region. This change allows usage of local servers.
Verification:
SLE12-SP2: http://10.100.12.105/tests/1105#step/libreoffice_pyuno_bridge/22
SLE12-SP3: http://10.100.12.105/tests/1104#step/libreoffice_pyuno_bridge/22
This is simple inclusion of already done work. Is possible to do like this?