Found 4 warning messages in watchdog that showed up after loading the page admin/config/development/testing:

Strict warning: Declaration of WebformSubmissionTestCase::setUp() should be compatible with WebformTestCase::setUp($added_modules = Array) in _registry_check_code() (line 176 of sites\all\modules\contrib\webform\tests\submission.test).

Strict warning: Declaration of WebformPermissionsTestCase::setUp() should be compatible with WebformTestCase::setUp($added_modules = Array) in _registry_check_code() (line 67 of sites\all\modules\contrib\webform\tests\permissions.test).

Strict warning: Declaration of WebformConditionalsTestCase::setUp() should be compatible with WebformTestCase::setUp($added_modules = Array) in _registry_check_code() (line 182 of sites\all\modules\contrib\webform\tests\conditionals.test).

Strict warning: Declaration of WebformComponentsTestCase::setUp() should be compatible with WebformTestCase::setUp($added_modules = Array) in require_once() (line 10 of sites\all\modules\contrib\webform\tests\components.test).
CommentFileSizeAuthor
#25 Remove-cruft-testdependency-make-sure-token-is-avail.patch3.74 KBhass
#23 Remove-cruft-testdependency-make-sure-token-is-avail.patch2.03 KBhass
#17 webform-test_warnings-2306521-17.patch2.03 KBdanchadwick
#17 webform_test_without_token.png15.01 KBdanchadwick
#8 Issue-2306521-by-hass-Add-optional-token-dependency-.patch1.16 KBhass

Comments

hass’s picture

hass commented

Aehm, what is this code supposed to do?

  /**
   * Implements setUp().
   */
  function setUp($added_modules = array()) {
    // Enable Webform and Token module if available.
    if (module_exists('token')) {
      $modules = array('webform', 'token');
    }
    else {
      $modules = array('webform');
    }
    parent::setUp(array_merge($modules, $added_modules));

Are you aware how to load optional modules for QA testing? You can add test_dependencies to your info file. This will download token module when a test is running on d.o, but your module will not depend on token! Hope this helps... :-)

[webform.info]
test_dependencies[] = token
danchadwick’s picture

danchadwick commented

A patch would be awesome. ;)

danchadwick’s picture

danchadwick commented

@hass -- can you post a patch? Your help would be appreciated!!

danchadwick’s picture

danchadwick commented
Status: Active » Fixed
Related issues: +#2244979: Figure out why testbot is failing all patches

The declaration of setup has been updated. Since we can't get dependencies, much less test_dependencies to work in the .info file, I'm marking this as closed, with a reference to the issue we are having with the testbot.

If someone with expertise here could help with a patch in #2244979: Figure out why testbot is failing all patches, that sure would be awesome.

hass’s picture

hass commented
Status: Fixed » Active

I think the other case is a duplicate of this patch here. At least #1 has the code for a propper patch that the other case has for sure NOT.

If I get an answer to my questions in #1 - I can share a patch.

danchadwick’s picture

danchadwick commented
Status: Active » Fixed

#1 has already been committed. There are no more strict warnings.

There are two questions in #1.

Aehm, what is this code supposed to do?

I didn't write it, but I infer that it intends to enable webform and, if available on the testbot, token.

Are you aware how to load optional modules for QA testing?

No.

Please feel free to post a patch or help out in #2244979: Figure out why testbot is failing all patches.

hass’s picture

hass commented
Status: Fixed » Active

#1 has been committed? Where? I noted that the code in #1 is WRONG. And added the 1 liner fix there. This has not committed and the broken code also need to be fixed.

Do not close cases you do not understand, please.

hass’s picture

hass commented
Version: 7.x-4.0-rc4 » 7.x-4.x-dev
Status: Active » Reviewed & tested by the community
StatusFileSize
new Issue-2306521-by-hass-Add-optional-token-dependency-.patch1.16 KB

Please note that test_dependency requires at least 6 or 12 hours to be committed before d.o testbot use this setting. At least in the past...

This is not testable with the testbot and requires a "blind" commit.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: Issue-2306521-by-hass-Add-optional-token-dependency-.patch, failed testing.

  • DanChadwick committed 4795e1f on 7.x-4.x 
    Issue #2306521 by hass: Added test_dependency on token.
danchadwick’s picture

danchadwick commented
Status: Needs work » Fixed

Thank you for the patch. It removes the ability to test when token is missing from the environment, which I believe to be intentional. I committed the test_dependency to .info.

hass’s picture

hass commented
Status: Fixed » Reviewed & tested by the community

Commit the rest, too. This is a bug.

danchadwick’s picture

danchadwick commented
Status: Reviewed & tested by the community » Fixed

The tests behave differently depending upon whether token is available at the time of the test. In the commit in which this was added (a66f61f), there is a test for token within the test itself. Therefore, I presume the intention on quicksketch is to allow the test to run with or without token, such that the optional support for token can be tested.

Thank you for your efforts in this issue. Assuming that tests continue to run once the d.o infrastructure catches up with what I committed in #10, this issue is closed. I will not remove the flexibility to test with and without token.

hass’s picture

hass commented
Status: Fixed » Needs review

I'm sure that code was added as he was not aware of test_dependency. token is optional and was therefore not downloaded by the test bot.

Now token is always available as test_dependency has been added and the condition is no longer necessary. This was by 99% a workaround to get the tests passing. As this is no longer required this useless code can be removed and should be removed.

danchadwick’s picture

danchadwick commented
Status: Needs review » Fixed

No, token is not always available by test_dependency. My reading of the commit that added support for test_dependency is that such modules become recommended, not required for simpletest. Therefore if token is not present in the test environment, the tests will continue to run, allowing tests to test the functionality of webform both with and without token installed.

Feel free to contact quicksketch offline. As maintainer, I am closing this issue. If quicksketch feels that the issue should be handled differently, he can do so. You are not the maintainer. Do not reopen.

hass’s picture

hass commented
Status: Fixed » Needs review

What? You do not understand how this works.

If test_dependency is set, the testbot will ALWAYS download token. Therefore the situation that the tests are running and token is not available does not exists any longer.

danchadwick’s picture

danchadwick commented
Status: Needs review » Fixed
StatusFileSize
new webform_test_without_token.png15.01 KB
new webform-test_warnings-2306521-17.patch2.03 KB

First, my apologies. I see that the derived webform boilerplate setup methods still had the old signature, throwing strict warnings. Committed to 7.x-4.x and 8.x.

If test_dependency is set, the testbot will ALWAYS download token. Therefore the situation that the tests are running and token is not available does not exists any longer.

You have not thought about local testing environments.
webform without token

What? You do not understand how this works.

It ain't what you don't know that gets you into trouble. It's what you know for sure that just ain't so. -- Mark Twain

hass’s picture

hass commented
Status: Fixed » Needs work

If you do not have token installed the tests must fail. Otherwise testing is useless to run tests. If you test a module you must test all features and not only a few.

Please commit my changes. Why is this bullsh** sooo a challenge here?

danchadwick’s picture

danchadwick commented
Status: Needs work » Closed (won't fix)

<summoning all my patience and courtesy>

Because you can write tests for when token is not available, since it is an optional module. And currently webform has such a contingency. Your change would prohibit this, reducing the possible test coverage for webform.

I have a valid reason, which I have tried to explain without success, and which I have demonstrated above. This is my decision to make. I appreciate your help in the issue queue. This issue is closed. Your trivial change, which makes zero difference to the testbot environment, but which removes additional testing options for local environments, will simply not be committed. Please accept this and move on to meaningful work.

I'm sure you use your discretion when dealing with issues in the modules you maintain. Please afford me the same courtesy.

</patience and courtesy>

  • DanChadwick committed f356151 on 7.x-4.x 
    Issue #2306521 by DanChadwick: Fixed strict warnings when testing.
  • DanChadwick committed 6582aa5 on 8.x-4.x
    Issue #2306521 by DanChadwick: Fixed strict warnings when testing.

  • DanChadwick committed 205322b on 8.x-4.x 
    Issue #2306521 by hass: Added test_dependency on token.
hass’s picture

hass commented

If you'd like to run a test once with and once without token you need two tests. Otherwise this cannot handled by the testbot. Local development is not what we write tests for. Testing is an automated process. It can run locally, but the intention is mostly for automated testing. Nothing else. Show me only one place in core that works this way, please.

hass’s picture

hass commented
Status: Closed (won't fix) » Needs review
StatusFileSize
new Remove-cruft-testdependency-make-sure-token-is-avail.patch2.03 KB

Like it or not. This patch changes document that the cruft should be removed once test_dependencies feature is available. This code may have been written before I asked for test_dependencies and testbot maintainers implemented this ~2 years ago.

Status: Needs review » Needs work

The last submitted patch, 23: Remove-cruft-testdependency-make-sure-token-is-avail.patch, failed testing.

hass’s picture

hass commented
Status: Needs work » Needs review
StatusFileSize
new Remove-cruft-testdependency-make-sure-token-is-avail.patch3.74 KB

Status: Needs review » Needs work

The last submitted patch, 25: Remove-cruft-testdependency-make-sure-token-is-avail.patch, failed testing.

danchadwick’s picture

danchadwick commented
Status: Needs work » Closed (won't fix)