Improve tests for gettext messages

Feature Ideas
1

We’ve found in our modules, and there have been cases in core modules too, that message.xml was not registered. It is also a very easy mistake to make to have a typo in a message id or to add a gettext call without adding it to message.xml.

So I propose we add a standard test for all modules that:

  • Ensures all messages referred by gettext() are registered in ir.message (this trivial to implement if we base our code on pygettext parser)
  • Ensure the text in ir.message contains the substitutions %(text)s and parameters in the gettext match (would be some more work, but probably doable extending the aforementioned parser)

Unfortunately we cannot raise an error if there are messages registered but not used in the module as one could want to register messages in a module for other modules to use, although, maybe we could add a special field for those (rare) cases.

Something in the line:

<record model="ir.message" id="msg_xxx">
    <field name="text">xxx</field>
    <field name="warn" eval="False"/>
</record>

Thoughts?

PS: It would also be nice to have a tool that ensures that all translations have the right substitutions, although I understand this is not desirable as a standard test because it could easily fail in core modules.

2

For me, it is not acceptable because the XML id could be constructed by code (as it is already in some cases).
And yes messages are also already reused from different modules.

3

What do you mean constructed by code? Can you put an example, please?

4

Like this:

raise Exception(gettext('msg_error_status_%s' % record.status))

But for the reused from different module, it should not be a problem as the other module should be a dependency.

I think we can have some improvement with testing.
We could have a generic test that check if ir.message id exists for ModelSQL._sql_constraint.
And we could have gettext raised error when running under test and log an error on normal run.

5

Exactly, it’s not a problem to ensure that all ids used in gettext() exist as ir.message. It is a problem the other way round: to ensure that all ir.message’s are really being used somewhere.

1 Like
6

What’s the problem of having a message not really used? It’s only an extra record in a database, I see no problem.

For me the problem is using a message that does not exist in the messages table as this will produce some messages that users does not understand. And this can be improved as follow:

Which should allow developers to spot this kind of errors and fix them.

7

I think you’ll be agreed it will be better not have any record unused in the database…

8

There are plenty of record unused in production database. It is not a problem and I do not think we should put energy in trying to solve this problem. From time to time, someone will find a record that can be removed and we will removed it.

1 Like
9

For me it was not a matter of having unnecessary records in the database but also avoiding unnecessary translations and don’t having code that is not necessary.

For me, it would be similar to the checks that there’s no “def default_x” method if there’s no “x” field. Although I understand that in the case of the methods could have undesired effects for inheriting modules.

But that is not a very important test, anyway.

1 Like
10

This is development management issue. We have the same problem with unused fields or method or even Model.
This can only be tackled by careful and constant review.

Anyway, I think the tests will be a good improvement with minimal code.

11

I don’t understand what you mean with this sentence. Do you mean that “gettext() if executed in tests and ir.message id does not exist it will raise an error”?

12

Yes, I mean that gettext should raise an error when run by tests if the id is missing. But we do not want to block usage of Tryton for such minor mistake.