Re: [Development] Duplicated test data tags
Mitch Curtis (14 October 2022 03:40) replied: >>> QTest::failOnWarning (introduced in 6.3) could also be used by tests >>> to make that warning fail the test: >>> >>> https://doc.qt.io/qt-6/qtest.html#failOnWarning Edward Welbourne (Friday, 14 October 2022 4:56 PM) answered: >> True enough; but you would need to add that to the start of your >> _data() function; I think a global setting is more in line with >> what's needed. Mitch Curtis (17 October 2022 02:59) asked: > So even init() would be too late? I would have thought that would run > before the data function. It's only run before the test itself - from invokeTestOnData(), which is what we loop over per call to the test function after we've built the data table. Eddy. ___ Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Duplicated test data tags
> -Original Message- > From: Edward Welbourne > Sent: Friday, 14 October 2022 4:56 PM > To: Milian Wolff ; EXT Mitch Curtis > Cc: Development@qt-project.org > Subject: Re: [Development] Duplicated test data tags [...] > Mitch Curtis (14 October 2022 03:40) replied: > > QTest::failOnWarning (introduced in 6.3) could also be used by tests > > to make that warning fail the test: > > > > https://doc.qt.io/qt-6/qtest.html#failOnWarning > > True enough; but you would need to add that to the start of your _data() > function; I think a global setting is more in line with what's needed. So even init() would be too late? I would have thought that would run before the data function. > Fortunately we do have QT_FATAL_WARNINGS; I had forgotten about that. > > So the question is: does it suffice to encourage developers to test with that > enabled, or do we need something more specific to the particular issue of tag > and column names in _data() functions ? > > Eddy. ___ Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Duplicated test data tags
Milian Wolff (Friday, 14 October 2022 3:00 AM) wrote:
>> >> I have many times accidentally written bogus code that duplicated the
>> >> tags. Getting a warning is useful, so thanks for working on that!
>> >>
>> >> But we won't easily spot these in the thousands of lines of outputs a
>> >> large test suite is generating.
On Freitag, 14. Oktober 2022 10:55:54 CEST Edward Welbourne wrote:
>> Indeed, that strikes me as eminently plausible.
>> Especially as one of the things I've been picking up on (and I've fixed
>> various of them, at least in QtCore) as a result of chasing these down
>> is that we have quite a few tests with QWARN messages, that really
>> should be anticipated (so that the test fails if the warning isn't
>> produced; and so that the test log isn't cluttered with the warnings)
>> using QTest::ignoreWarning().
>>
>> I should also take this opportunity to encourage all developers to watch
>> out for QWARN messages in test output: if it's unexpected, it may be the
>> symptom of a lurking bug; otherwise, its presence should be tested for,
>> see preceding. If the code under test fails to produce a warning that
>> was expected, that's an issue the test should catch.
>> >> At the very least I would suggest something akin to QT_FATAL_WARNINGS
>> >> that can be set to more easily spot faulty client code.
>>
>> Perhaps, given the above, we should just encourage developers to use
>> QT_FATAL_WARNINGS more often ?
>>
>> I do note, however, that it seems not to have been documented; I shall
>> add a doc update to my branch.
Milian Wolff (Friday, October 14, 2022 12:29) replied:
> That is sadly not an option in many situations. The silencing of the warnings
> does not influence the fatal-abort to my knowledge? See
> `QMessageLogger::warning`:
>
> ```
> void QMessageLogger::warning(const char *msg, ...) const
> {
> va_list ap;
> va_start(ap, msg); // use variable arg list
> const QString message = qt_message(QtWarningMsg, context, msg, ap);
> va_end(ap);
>
> if (isFatal(QtWarningMsg))
> qt_message_fatal(QtWarningMsg, context, message);
> }
> ```
That sounds like something we should look into changing. It runs
counter to what QTest::ignoreWarnings() tells its user; they may
reasonably expect setting the environment variable to catch only
warnings they haven't suppressed. However, that's very much within
QMessageLogger code, not under QTest's control :-(
QTBUG-107659
> So while `qt_message` is intercepted, the `qt_message_fatal` is not which
> then
> would kill the app even for "expected" warnings.
:-(
> What's worse, there are many warnings where you don't know how many of them
> will occur. We e.g. run our unit test suite using the offscreen platform
> plugin and are drowning in warnings such as:
>
> ```
> This plugin does not support setParent!
> ```
>
> And so far I could not find a reliable way to prevent this.
QTBUG-107660
> Similarly, we need a custom OpenGL profile which always triggers this warning
> when Qt WebEngine is also used:
>
> ```
> An OpenGL Core Profile was requested, but it is not supported on the current
> platform. Falling back to a non-Core profile. Note that this might cause
> rendering issues.
> ```
>
> Meaning: there are sadly some situations where warnings are emitted that you
> do not have control over. Meaning, Qt_FATAL_WARNINGS is sadly not an option
> today :(
Sounds painful.
Thanks for the dose of real-world practicality. I'll need to give
further thought to how to make the new warnings "enforceable"
(without enforcing them all the time).
Eddy.
___
Development mailing list
Development@qt-project.org
https://lists.qt-project.org/listinfo/development
Re: [Development] Duplicated test data tags
On Freitag, 14. Oktober 2022 10:55:54 CEST Edward Welbourne wrote:
> Milian Wolff (Friday, 14 October 2022 3:00 AM)
>
> >> I have many times accidentally written bogus code that duplicated the
> >> tags. Getting a warning is useful, so thanks for working on that!
> >>
> >> But we won't easily spot these in the thousands of lines of outputs a
> >> large test suite is generating.
>
> Indeed, that strikes me as eminently plausible.
> Especially as one of the things I've been picking up on (and I've fixed
> various of them, at least in QtCore) as a result of chasing these down
> is that we have quite a few tests with QWARN messages, that really
> should be anticipated (so that the test fails if the warning isn't
> produced; and so that the test log isn't cluttered with the warnings)
> using QTest::ignoreWarning().
>
> I should also take this opportunity to encourage all developers to watch
> out for QWARN messages in test output: if it's unexpected, it may be the
> symptom of a lurking bug; otherwise, its presence should be tested for,
> see preceding. If the code under test fails to produce a warning that
> was expected, that's an issue the test should catch.
>
> >> At the very least I would suggest something akin to QT_FATAL_WARNINGS
> >> that can be set to more easily spot faulty client code.
>
> Perhaps, given the above, we should just encourage developers to use
> QT_FATAL_WARNINGS more often ?
>
> I do note, however, that it seems not to have been documented; I shall
> add a doc update to my branch.
That is sadly not an option in many situations. The silencing of the warnings
does not influence the fatal-abort to my knowledge? See
`QMessageLogger::warning`:
```
void QMessageLogger::warning(const char *msg, ...) const
{
va_list ap;
va_start(ap, msg); // use variable arg list
const QString message = qt_message(QtWarningMsg, context, msg, ap);
va_end(ap);
if (isFatal(QtWarningMsg))
qt_message_fatal(QtWarningMsg, context, message);
}
```
So while `qt_message` is intercepted, the `qt_message_fatal` is not which then
would kill the app even for "expected" warnings.
What's worse, there are many warnings where you don't know how many of them
will occur. We e.g. run our unit test suite using the offscreen platform
plugin and are drowning in warnings such as:
```
This plugin does not support setParent!
```
And so far I could not find a reliable way to prevent this.
Similarly, we need a custom OpenGL profile which always triggers this warning
when Qt WebEngine is also used:
```
An OpenGL Core Profile was requested, but it is not supported on the current
platform. Falling back to a non-Core profile. Note that this might cause
rendering issues.
```
Meaning: there are sadly some situations where warnings are emitted that you
do not have control over. Meaning, Qt_FATAL_WARNINGS is sadly not an option
today :(
> Mitch Curtis (14 October 2022 03:40) replied:
> > QTest::failOnWarning (introduced in 6.3) could also be used by tests
> > to make that warning fail the test:
> >
> > https://doc.qt.io/qt-6/qtest.html#failOnWarning
>
> True enough; but you would need to add that to the start of your _data()
> function; I think a global setting is more in line with what's needed.
> Fortunately we do have QT_FATAL_WARNINGS; I had forgotten about that.
>
> So the question is: does it suffice to encourage developers to test with
> that enabled, or do we need something more specific to the particular
> issue of tag and column names in _data() functions ?
>
> Eddy.
--
Milian Wolff
m...@milianw.de
http://milianw.de
signature.asc
Description: This is a digitally signed message part.
___
Development mailing list
Development@qt-project.org
https://lists.qt-project.org/listinfo/development
Re: [Development] Duplicated test data tags
Milian Wolff (Friday, 14 October 2022 3:00 AM) >> I have many times accidentally written bogus code that duplicated the >> tags. Getting a warning is useful, so thanks for working on that! >> >> But we won't easily spot these in the thousands of lines of outputs a >> large test suite is generating. Indeed, that strikes me as eminently plausible. Especially as one of the things I've been picking up on (and I've fixed various of them, at least in QtCore) as a result of chasing these down is that we have quite a few tests with QWARN messages, that really should be anticipated (so that the test fails if the warning isn't produced; and so that the test log isn't cluttered with the warnings) using QTest::ignoreWarning(). I should also take this opportunity to encourage all developers to watch out for QWARN messages in test output: if it's unexpected, it may be the symptom of a lurking bug; otherwise, its presence should be tested for, see preceding. If the code under test fails to produce a warning that was expected, that's an issue the test should catch. >> At the very least I would suggest something akin to QT_FATAL_WARNINGS >> that can be set to more easily spot faulty client code. Perhaps, given the above, we should just encourage developers to use QT_FATAL_WARNINGS more often ? I do note, however, that it seems not to have been documented; I shall add a doc update to my branch. Mitch Curtis (14 October 2022 03:40) replied: > QTest::failOnWarning (introduced in 6.3) could also be used by tests > to make that warning fail the test: > > https://doc.qt.io/qt-6/qtest.html#failOnWarning True enough; but you would need to add that to the start of your _data() function; I think a global setting is more in line with what's needed. Fortunately we do have QT_FATAL_WARNINGS; I had forgotten about that. So the question is: does it suffice to encourage developers to test with that enabled, or do we need something more specific to the particular issue of tag and column names in _data() functions ? Eddy. ___ Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Duplicated test data tags
> -Original Message- > From: Development On Behalf Of > Milian Wolff > Sent: Friday, 14 October 2022 3:00 AM > To: Development@qt-project.org > Subject: Re: [Development] Duplicated test data tags > [...] > I have many times accidentally written bogus code that duplicated the tags. > Getting a warning is useful, so thanks for working on that! > > But we won't easily spot these in the thousands of lines of outputs a large > test suite is generating. At the very least I would suggest something akin to > QT_FATAL_WARNINGS that can be set to more easily spot faulty client code. QTest::failOnWarning (introduced in 6.3) could also be used by tests to make that warning fail the test: https://doc.qt.io/qt-6/qtest.html#failOnWarning > Cheers > -- > Milian Wolff > m...@milianw.de > http://milianw.de ___ Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Duplicated test data tags
On Donnerstag, 13. Oktober 2022 15:50:06 CEST Edward Welbourne via Development wrote: > QTBUG-107185 revealed that QTest did not check for duplicated test data > tags, i.e. parameters to newRow() / addRow(); when I looked at the > implementation I found it also neglected to check for duplicated > addColumn names. So [0] sets out to fix that. > > It turns out that Qt itself has "quite a lot" of instances of duplicate > test data tags. (I've only found one duplicate column, thankfully; but > there may be more.) So, for now, the fix merely produces a warning; and > I do encourage all to watch out for "Duplicate data tag ... - please > rename." messages in test output (once [0] has landed). Likewise for > similar with "column" in place of "tag". > > [0] https://codereview.qt-project.org/c/qt/qtbase/+/436495 > > You can see from the branch it's the tip of, currently, some of the many > cases of duplication we had before it brought them to my attention. > There remain more even in qtbase and I haven't (for now, at least) > looked further afield. > > The present draft of that change has #if-ery to turn duplication of > column or row names, in QTest data tables, at Qt 7, into a fatal error. > How realistic do folk think it is that client code authors will respond > to the warnings in time for that ? > > I have, for now, included #if-ery complications that let you, when > building QtTestLib, define Q_TEST_REJECT_DUPLICATE_ROW to 0 or 1 in the > build system (and likewise for ..._COLUMN) to override the default > behaviour (0 means warning only even for Qt 7; 1 means fatal even before > Qt 7) but without any build infrastructure (yet) to set that. Is this > adequate or overkill; does anyone have a better suggestion ? I have many times accidentally written bogus code that duplicated the tags. Getting a warning is useful, so thanks for working on that! But we won't easily spot these in the thousands of lines of outputs a large test suite is generating. At the very least I would suggest something akin to QT_FATAL_WARNINGS that can be set to more easily spot faulty client code. Cheers -- Milian Wolff m...@milianw.de http://milianw.de signature.asc Description: This is a digitally signed message part. ___ Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Duplicated test data tags
On Thursday, 13 October 2022 06:50:06 PDT Edward Welbourne via Development wrote: > The present draft of that change has #if-ery to turn duplication of > column or row names, in QTest data tables, at Qt 7, into a fatal error. > How realistic do folk think it is that client code authors will respond > to the warnings in time for that ? > > I have, for now, included #if-ery complications that let you, when > building QtTestLib, define Q_TEST_REJECT_DUPLICATE_ROW to 0 or 1 in the > build system (and likewise for ..._COLUMN) to override the default > behaviour (0 means warning only even for Qt 7; 1 means fatal even before > Qt 7) but without any build infrastructure (yet) to set that. Is this > adequate or overkill; does anyone have a better suggestion ? We can flip the default in Qt 7 and also make it a test option that QTEST_MAIN picks up on for code that is too much effort to fix. -- Thiago Macieira - thiago.macieira (AT) intel.com Cloud Software Architect - Intel DCAI Cloud Engineering ___ Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
