backdrop
Simpletest semi-silently ignores wrong file paths in .tests.info file
As noted in https://github.com/backdrop/backdrop-issues/issues/5490,
it turned out that several tests actually never run: core/modules/system/tests/system.tests.info contains wrong file paths It's still not clear, why these tests ran in the past, but don't for several weeks. Eventually related: Issue https://github.com/backdrop/backdrop-issues/issues/5415: Gracefully handle empty or malformed .tests.info files.
In https://github.com/backdrop/backdrop-issues/issues/5415, we introduced code that checked for malformed .tests.info files and put errors into watchdog when they're detected. We can (should) add to this error-checking to verify that the file paths specified in the tests.info file are correct.
PR to follow.
The PR can be tested with the existing code base (prior to merging PR https://github.com/backdrop/backdrop/pull/3942) by:
- Enabling Simpletest
- Visiting the testing page at admin/config/development/testing
- Going to watchdog at admin/reports/dblog and examining the most recent entry.
@bugfolder many thanks for the PR, the code looks good.
I do have two concerns, though:
- The watchdog message happens only once (simpletest caches the result), so this warning may be overlooked, unless caches get flushed often.
- Besides that log message, there's now absolutely nothing that indicates a problem. Via UI tests simply don't appear, in command line runs tests aren't listed, either.
Your PR hides problems even more. For instance, in our GHA nobody would ever realize that something's broken, because what previously caused errors is now in complete stealth mode. That seems a bit dangerous to me.
Indeed, thank you @bugfolder ...but I'm also with @indigoxela on this: we need to be more "verbose" when there are wrong paths in test files. I'd suggest at least the following:
- when the simpletest module is enabled if test files with erroneous file paths are detected, there's an error shown in the test selection page at
admin/config/development/testing(and perhaps also on the site status page?). - we make it so that tests fail hard when test file paths are wrong (test files missing)
@klonos yes, that would make sense IMO :+1:
A quick idea regarding command line: we have function backdrop_is_cli(), which we can use to let tests fail for cli, but only show a descriptive error message on the selection page when using UI, still suppressing fatal errors. Not sure how easy/complex that would be.
- Agreed that stronger notification is needed. Seems like any of the error states detected in
simpletest_get_all()would warrant strong notification, not just the bad file path error. - An easy way to highlight problems in the UI would be to post the same error message using
backdrop_set_message()at the same time as thewatchdog()entry. Reasonable? - We could ensure that the check is always run when viewing the UI if we cleared the static variable in
simpletest_test_form(), but that would slow down form loading. Is that an acceptable price to pay (considering that the vast majority of the time there won't be any errors)? - using
backdrop_is_cli()we could throw a failed test assertion instead of posting messages and watchdog, but there's other reasons to run backdrop from the CLI (e.g., cron calls) that wouldn't warrant a hard fail. Perhaps testingsetUp()routine could post a global variable$_running tests(maybe a static variable in BackdropWebTestCase), or something like that, that could be probed to know whether tests are running.
An easy way to highlight problems in the UI would be to post the same error message using backdrop_set_message() at the same time as the watchdog() entry. Reasonable?
Yes, makes sense. Hm... regarding the static cache - I'm not sure if I understand what you're after.
using backdrop_is_cli() we could throw a failed test assertion instead
This might be a bit tricky (the assertion part), because the error already happens when collecting data and the test class itself isn't loaded. But we could do something easier like throwing an Exception. Then it would (should?) be visible in the GHA and also when running it locally via cli. And people could get the location and name of the failing test.info file.
throw new Exception('Some helpful hint with path and name here.');
My main concern is that currently it might appear as if "everything's green", although there's a severe problem. An exception could be enough to indicate "a problem needs to get addressed". There may be other opinions, though.
...but there's other reasons to run backdrop from the CLI (e.g., cron calls) that wouldn't warrant a hard fail
I don't actually think that a cron run triggers function simpletest_test_get_all().
I don't actually think that a cron run triggers function simpletest_test_get_all().
Doi!
regarding the static cache - I'm not sure if I understand what you're after.
Sorry, not the static cache, the db cache (the cache()->set('simpletest', $groups) line in simpletetest_get_all()). If you refresh the testing listing page without clearing the cache, the test code doesn't run because the page is using the cached values.
Doi!
Huh? :upside_down_face:
not the static cache, the db cache (the cache()->set('simpletest', $groups)
Ah, OK, now I get it. :smile: Hm... I'd prefer if we find a solution without removing that cache. Not sure what would be smart here.
Doi! Huh?
Sorry, an Americanism. It means, roughly, "I should have realized that if I had two neurons to rub together." 😉
I'd prefer if we find a solution without removing that cache. Not sure what would be smart here.
Maybe we start by trying it without explicitly clearing the cache. If people are doing development, there's probably some cache-clearing going on, and that's when they'd most need to catch typos in the file path (or other malformations).
Well, that was suprising. If I just throw an exception in simpletest_get_all(), the attempted test run fails, but shows up green in the PR. Not what we want.
Huh? Seems like although there's an exception, the exit code of run-tests.sh is still 0? :confused: And GHA interprets this 0 as success...
Can we try something like that instead?
print "$error\n";
exit(1);
Just to see how that shows up in the action.
Much better. (I also eliminated the HTML formatting (@uri versus %uri) from the error messages to make them usable both in the UI and as GHA error messages.)
@bugfolder many thanks for updating. And removing the formatting makes messages much easier to read, when running on command line (both, local/manual and in GHA).
No more hidden-away errors - wrong file paths in info files are now loud enough. Works for me. :+1:
I had another idea re errors - doing it like function assert and put the individual test failures in the database, but didn't dig deeper. IMO that would be a waste of resources, as at that point we already know, the tests have to fail. Running the remaining (still correct) tests would be a waste of time and CPU.
@indigoxela, @klonos, either of you up for a code review to get this to RTBC?
I don't think, I should do the code review here - got too much involved in the current solution. Another pairs of eyes would be appreciated.
I'm not sure about the PR... In some places $error is a plain string, in others it uses the t() function, and in yet others it uses format_string(). Finally, at the end, it seems that regardless of how it was set above, it's then passed through format_string()...
Am I reading it wrong?
... plain string, ... the t() function, ... format_string()
Not to forget format_plural(). :wink:
TBH I never completely understood, when to translate a string in simpletest and when not.
Re plain string vs. format_string - the latter is used when it's necessary to insert variables in a string. It won't get translated. format_plural does translate, though.
I find this posting very helpful. In a nutshell, only use t when testing translations, or when you want to match UI text that may be translated.
https://www.drupal.org/docs/7/testing/simpletest-testing-tutorial-drupal-7#t
That post advises using format_string instead when formatting and sanitizing.
So, although Simpletest module is all about testing, the error messages in this PR are not "messages that result from running a test," but rather are messages that the user sees in the UI (via backdrop_set_message() &/or watchdog()) when they're deciding what tests to run. So the messages in this PR should be translated.
And it's a little confusing because format_string() explicitly doesn't run its string arguments through t(), but format_plural() does. So in this PR, a string destined for format_string() should have been run through t(), but we can (and should) pass "raw" strings to format_plural(), which will do its own t()-ifying.
With all that being said, @BWPanda did indeed catch an inconsistency: I missed the very first case (it needed t()), now fixed in the PR.
@indigoxela, @klonos, @argiepiano, any of you up for a code review of this?
@bugfolder I've left some questions. It's not about newly introduced things, but pre-existing.
@indigoxela, thanks for the review! I added some explanation to your first comment; more details are in my comment above.
I closed and reopened the PR, and now PHPCS is complaining about entirely unrelated files (entityreference module), so I'm going to ignore that for now.
Rebased the PR. I also added some comments and code changes to make clearer the delayed substitution of variables (still using '@' rather than '%' for clarity on the command line and in GHA messages).
@indigoxela, I think the PHPCS issues are being dealt with in your new issue (thanks!); would you (&/or @argiepiano) take a look at the new PR on this issue and see if the changes address your concerns about splitting up the variable substitutions (they're now applied all in one place)?