ManageIQ / manageiq

ManageIQ Open-Source Management Platform
https://manageiq.org
Apache License 2.0
1.35k stars 900 forks source link

Remove noisy test output logging we've never cared about #23213

Closed jrafanie closed 1 month ago

jrafanie commented 2 months ago

Technically, we could try to fix this and get the translators to update their data to have this fixed, but it's possible there are situations where this happens again and it's not an actual problem for gettext.

Even the original PR that added this said this overall number of newlines check could flag intentionally changed values. Since we've never reacted to this output, we can remove it.

From that PR (we care about the 3rd check below):

1. if original string starts with a newline, translation needs to start with a newline too
2. if original string ends with a newline, translated string also needs to end with a newline
3. overal amount of newlines in original and translated string needs to match

In this PR, I am introducing a new test, which tests the above rules. The exception is the 3rd rule, since it seems that in some places the newline might have been added (or omitted) intentionaly. So we're just logging these.

Reference:: https://github.com/ManageIQ/manageiq/pull/20815

jrafanie commented 2 months ago

FYI, this is the output we're removing:

The following translation entries do not honor overall number of newlines:
>> File: /home/runner/work/manageiq/manageiq/locale/es/manageiq.po
----------
»VMs must be scanned from an EVM server whose host is attached to the same
  storage as the VM unless overridden via SmartProxy affinity.
  Please verify that:
  1) Direct LUNs are attached to ManageIQ appliance
  2) Management Relationship is set for the ManageIQ appliance«
»Se deben analizar las MV desde un servidor EVM cuyo host esté conectado al mismo almacenamiento que la MV, a menos que se anule mediante afinidad con SmartProxy.
  Compruebe que:
  1) Los LUN directos estén conectados a un dispositivo ManageIQ
  2) La relación de gestión esté configurada para el dispositivo ManageIQ«
----------

>> File: /home/runner/work/manageiq/manageiq/locale/it/manageiq.po
----------
»VMs must be scanned from an EVM server whose host is attached to the same
  storage as the VM unless overridden via SmartProxy affinity.
  Please verify that:
  1) Direct LUNs are attached to ManageIQ appliance
  2) Management Relationship is set for the ManageIQ appliance«
»Le VM devono essere scansate da un server EVM il cui host è collegato alla stessa memoria della VM a meno che non sovrascrivi tramite affinità SmartProxy.
  Si prega di verificare che: 1) I LUN diretti siano collegati al dispositivo ManageIQ 2) La Relazione di gestione sia impostata per il dispositivo ManageIQ«
----------

>> File: /home/runner/work/manageiq/manageiq/locale/pt_BR/manageiq.po
----------
»Enables defining a URL path prefix for XCCDF file instead of accessing the default location.
  example: http://my_file_server.org:[33](https://github.com/ManageIQ/manageiq/actions/runs/11073121485/job/30776894202?pr=23204#step:8:34)33/xccdf_files/
  Expecting to find com.redhat.rhsa-RHEL7.ds.xml.bz2 file there.«
»Permite definir um prefixo de caminho de URL para o arquivo XCCDF em vez de acessar o local padrão.
  Por exemplo: http://my_file_server.org: 3333/xccdf_files/ Esperando encontrar o arquivo com.redhat.rhsa-RHEL7.ds.xml.bz2 lá.«
----------
»Enables defining a URL path prefix for XCCDF file instead of accessing the default location.
example: http://my_file_server.org:3333/xccdf_files/
Expecting to find com.redhat.rhsa-RHEL7.ds.xml.bz2 file there.«
»Permite definir um prefixo de caminho de URL para o arquivo XCCDF em vez de acessar o local padrão.
Por exemplo: http://my_file_server.org: 3333/xccdf_files/ Esperando encontrar o arquivo com.redhat.rhsa-RHEL7.ds.xml.bz2 lá.«
----------
»Provision failed for the following reasons:
%{errors}«
»A provisão falhou pelos seguintes motivos: %{errors}«
----------
»VMs must be scanned from an EVM server whose host is attached to the same
  storage as the VM unless overridden via SmartProxy affinity.
  Please verify that:
  1) Direct LUNs are attached to ManageIQ appliance
  2) Management Relationship is set for the ManageIQ appliance«
»As VMs devem ser escaneadas a partir de um servidor EVM cujo host esteja conectado ao mesmo armazenamento que a VM, a menos que seja substituído por meio da afinidade do SmartProxy.
  Verifique se: 1) LUNs diretas estão conectadas ao dispositivo ManageIQ
2) O relacionamento de gerenciamento está configurado para o dispositivo ManageIQ«
----------

>> File: /home/runner/work/manageiq/manageiq/locale/zh_CN/manageiq.po
----------
»Enables defining a URL path prefix for XCCDF file instead of accessing the default location.
  example: http://my_file_server.org:3333/xccdf_files/
  Expecting to find com.redhat.rhsa-RHEL7.ds.xml.bz2 file there.«
»允许为 XCCDF 文件定义 URL 路径前缀,而不是访问缺省位置。示例:http://my_file_server.org:3333/xccdf_files/
  应该可在那里找到 com.redhat.rhsa-RHEL7.ds.xml.bz2 文件。«
----------
»Enables defining a URL path prefix for XCCDF file instead of accessing the default location.
example: http://my_file_server.org:3333/xccdf_files/
Expecting to find com.redhat.rhsa-RHEL7.ds.xml.bz2 file there.«
»允许为 XCCDF 文件定义 URL 路径前缀,而不是访问缺省位置。示例:http://my_file_server.org:3333/xccdf_files/
  应该可在那里找到 com.redhat.rhsa-RHEL7.ds.xml.bz2 文件。«
----------
»Provision failed for the following reasons:
%{errors}«
»由于以下原因,供应失败:%{errors}«
----------
»VMs must be scanned from an EVM server whose host is attached to the same
  storage as the VM unless overridden via SmartProxy affinity.
  Please verify that:
  1) Direct LUNs are attached to ManageIQ appliance
  2) Management Relationship is set for the ManageIQ appliance«
»虚拟机必须从 EVM 服务器扫描,它的主机附加到和虚拟机相同的存储器上,除非通过 SmartProxy 关联覆盖。
  请验证:
  1) 直接 LUN 附加到 ManageIQ 设备
  2) 为 ManageIQ 设备设置了管理关系«
----------

>> File: /home/runner/work/manageiq/manageiq/locale/zh_TW/manageiq.po
----------
»Datastore import was successful.
Namespaces updated/added: %{namespace_stats}
Classes updated/added: %{class_stats}
Instances updated/added: %{instance_stats}
Methods updated/added: %{method_stats}«
»成功匯入資料儲存庫。已更新/已新增名稱空間:%{namespace_stats}
已更新/已新增類別:%{class_stats}
已更新/已新增實例:%{instance_stats}
已更新/已新增方法:%{method_stats}«
----------
»Enables defining a URL path prefix for XCCDF file instead of accessing the default location.
  example: http://my_file_server.org:3333/xccdf_files/
  Expecting to find com.redhat.rhsa-RHEL7.ds.xml.bz2 file there.«
»啟用為 XCCDF 檔案定義 URL 路徑字首而非存取預設位置的功能。例如:http://my_file_server.org:3333/xccdf_files/,期望在這裡能夠找到 com.redhat.rhsa-RHEL7.ds.xml.bz2 檔。«
----------
»Enables defining a URL path prefix for XCCDF file instead of accessing the default location.
example: http://my_file_server.org:3333/xccdf_files/
Expecting to find com.redhat.rhsa-RHEL7.ds.xml.bz2 file there.«
»啟用為 XCCDF 檔案定義 URL 路徑字首而非存取預設位置的功能。例如:http://my_file_server.org:3333/xccdf_files/,期望在這裡能夠找到 com.redhat.rhsa-RHEL7.ds.xml.bz2 檔。«
----------
»Provision failed for the following reasons:
%{errors}«
»供應失敗,原因如下:%{errors}«
----------
»VMs must be scanned from an EVM server whose host is attached to the same
  storage as the VM unless overridden via SmartProxy affinity.
  Please verify that:
  1) Direct LUNs are attached to ManageIQ appliance
  2) Management Relationship is set for the ManageIQ appliance«
»除非透過 SmartProxy 親緣性置換,否則必須從其主機連接至與 VM 相同的儲存體的 EVM 伺服器中掃描 VM。
  請驗證:  1) 直接 LUN 已連接至 ManageIQ 應用裝置  2) 已為 ManageIQ 應用裝置設定管理關係«
----------
miq-bot commented 2 months ago

Checked commit https://github.com/jrafanie/manageiq/commit/e590ce0b909a020ad266f75b462c041cc8d1c50a with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint 1 file checked, 0 offenses detected Everything looks fine. :star:

Fryguy commented 2 months ago

While I don't like the noise, this test was here for a reason and that's that the translators are incorrectly translating. So, taking this for an example:

»VMs must be scanned from an EVM server whose host is attached to the same
  storage as the VM unless overridden via SmartProxy affinity.
  Please verify that:
  1) Direct LUNs are attached to ManageIQ appliance
  2) Management Relationship is set for the ManageIQ appliance«
»除非透過 SmartProxy 親緣性置換,否則必須從其主機連接至與 VM 相同的儲存體的 EVM 伺服器中掃描 VM。
  請驗證:  1) 直接 LUN 已連接至 ManageIQ 應用裝置  2) 已為 ManageIQ 應用裝置設定管理關係«

That's showing me that the 1) and 2) are inline, so presentation wise it's bad...this is what the test is supposed to show and the goal was to either

  1. go back to the translators and say it's wrong
  2. fix the code to not use newlines and/or present it differently

I'd really rather not delete this test, because it does have value. Incidentally, if you look closely it's like only 2 strings at fault, so IMO we should just fix them.

Fryguy commented 2 months ago

I am curious where these are even being displayed in the UI. newlines are notoriously bad for UI because they don't necessarily render as a <br /> unless you make it happen, which makes me wonder if they are poorly presented in the first place.

kbrock commented 2 months ago

@Fryguy I see that we have a few possibilities:

  1. Continue to ignore the warnings
  2. Build a process to update these values on every run (chances are these same exact values come every time so a review and fix will work)
  3. Send the corrections to our translators and ask them to fix the source of this information.
  4. Throw away the tests.

Since we haven't done anything for the past 4 years, not sure how much we want to spend on a solution.

But the proper solution is number 3. Just not sure how to do this. The least expensive option is 4 - just have to click merge on that one.

Fryguy commented 2 months ago

Or my number 2

  1. fix the code to not use newlines and/or present it differently

Which, given there are literally only 3 strings in the list, seems like the simplest option.

jrafanie commented 2 months ago

I am curious where these are even being displayed in the UI. newlines are notoriously bad for UI because they don't necessarily render as a <br /> unless you make it happen, which makes me wonder if they are poorly presented in the first place.

yeah, I don't know. Is it worth the effort to track them down and fix?

kbrock commented 1 month ago

it seems that in some places the newline might have been added (or omitted) intentionally. So we're just logging these. -- #20815

jrafanie commented 1 month ago

My thinking on this PR is YAGNI. It's been a noisy log message since it's introduction. It was never worthy of failing tests so it was never that important to fail tests.

We've not cared about it until now so I figured we can add it back if we suddenly want to track these down and fix them.

Fryguy commented 1 month ago

This is an easy solve - here are the places that these strings live and none of them actually need newlines, so I'll just make PRs, and then we can leave this test. Also, I was wrong - it was 4 strings, not 3 😛


jrafanie commented 1 month ago

This is an easy solve - here are the places that these strings live and none of them actually need newlines, so I'll just make PRs, and then we can leave this test. Also, I was wrong - it was 4 strings, not 3 😛

If this was important why was it not failing the tests? Should I make this test fail and not just muddy the output when running the tests locally or in CI? I didn't want to spend any time on it since the comments in the PR and the code indicate it wasn't actually a problem all the time. But if all of the strings can be fixed and verified as working in the UI so it sounds like we should really fail the test and possibly offer exceptions if there ever is an exception.

Fryguy commented 1 month ago

I disagree with the initial assessment from Milan that sometimes it was necessary to change the newlines. In all of these cases, the translators just did it wrong. I agree with you that this test should fail, not warn, and that way we can actually catch these from importing the automated translations at the time they are imported.

In fact, adding newlines is a thing we don't want the translators to do cause that can actually break outputs, so this test is really important to find those.

It also happens that we don't need the newlines here even in English, so changing them seems like a simple solution as these strings will then just be resubmitted to the translation team.

jrafanie commented 1 month ago

I disagree with the initial assessment from Milan that sometimes it was necessary to change the newlines. In all of these cases, the translators just did it wrong. I agree with you that this test should fail, not warn, and that way we can actually catch these from importing the automated translations at the time they are imported.

I can followup and make it fail once they're fixed.

In fact, adding newlines is a thing we don't want the translators to do cause that can actually break outputs, so this test is really important to find those.

Yeah, now that we do scheduled runs of po/pot generation and failing this test in the future, we'll be picking any of these up shortly after it's committed.

It also happens that we don't need the newlines here even in English, so changing them seems like a simple solution as these strings will then just be resubmitted to the translation team.

Right, any solution should be in the english pot so we can get them translated as it's much easier to follow the normal pattern vs. asking them to manually update their existing data to manually update their translations.

jrafanie commented 1 month ago

Closing in favor of https://github.com/ManageIQ/manageiq-ui-classic/pull/9278 https://github.com/ManageIQ/manageiq/pull/23215 https://github.com/ManageIQ/manageiq-providers-ovirt/pull/677 and https://github.com/ManageIQ/manageiq-providers-kubernetes/pull/539

jrafanie commented 1 month ago

I'll open a PR to make this situation raise an error instead of flooding stdout and getting largely ignored.