The scheduler test module which implements one of our API hooks needs to be converted to 8.x before the tests can be re-worked. We could also expand the module to implement more of the hooks and then increase the test coverage.

I started this work in #2594615-114: Automated testing in 8.x [meta] but it will be easier to follow on a separate issue. Hence I will remove my original comments and patch and put them in this new issue.
CommentFileSizeAuthor
#36 2655666-36.avoid_side-effects_on_non_test_content.patch6.58 KBjonathan1055
#29 2655666-29.api_test_publish_immediately.patch2.53 KBjonathan1055
#27 2655666-27.api_test_changes_after_review.patch4.86 KBjonathan1055
#23 2655666-23.api_test_rest_of_hooks.patch18.23 KBjonathan1055
#21 2655666-21.api_test_module.patch16.6 KBjonathan1055
#15 2655666-15.api_test_module.patch20.05 KBjonathan1055
#12 2655666-12.api_test_module.patch20.8 KBjonathan1055
#10 2655666-10.api_test_module.patch15.92 KBjonathan1055
#8 API test module notes.pdf24.94 KBjonathan1055

Comments

jonathan1055 created an issue. See original summary.

  • jonathan1055 committed dc4718a on 8.x-1.x 
    Issue #2655666 by jonathan1055: Rename module scheduler_test to...

  • jonathan1055 committed 60cba69 on 8.x-1.x 
    Issue #2655666 by jonathan1055: Convert scheduler_api_test_install()
jonathan1055’s picture

jonathan1055 commented

In the commit in #2 I have renamed the module from scheduler_test to scheduler_api_test, so that it is clear and more specific what the module is for.

The commit in #3 works on hook_install(). Progress so far: determine if the custom node type exists, create it if not, adding a body field as before. Lists the fields that exist, and create the custom field 'approved' if it is not found. However, I have not managed to save the field definition permanently, it is only 'in code'.

jonathan1055’s picture

jonathan1055 commented
Status: Active » Needs review

I found that to do the conversion you need to be able to enable and run the test module. To avoid duplicating code, a simple solution is to create a folder scheduler_api_test in the main /modules directory, copy scheduler_api_test.info.yml to that folder as this file is relatively static, but instead of duplicating .install and .module, create two files which just include the actual source file. So modules/scheduler_api_test.install just contains:

include 'modules/scheduler/tests/modules/scheduler_api_test.install';

Likewise for .module. Then you can test the module interactively and via simpletest using exactly the same source code.

To show the latest state of the conversion, after enabling the module interactively, and hook_install() has run if you go to the status report page /admin/reports/status, you get a message:

Mismatched entity and/or field definitions
The following changes were detected in the entity type and field definitions.
Content - Delete the Approval field.

I tracked this down to \Drupal\Core\Entity\EntityDefinitionUpdateManager and it seems that because the field is not saved in storage but is defined in code then the implication is that an admin user has deleted the fields manually, and that is what the message above is trying to say. Executing \Drupal::service('entity.definition_update_manager')->applyUpdates(); removes the status warning, but that is no help, because we need to actually get the field saved in storage. That is where I am at currently, and would welcome any ideas on how to progress.

The code is very much work-in-progress, with plenty of devel module dd() calls and commented-out lines. I have not started working on the .module file yet, as that will depend on how the .install process is done.

  • jonathan1055 committed 1dc6cbb on 8.x-1.x 
    Issue #2655666 by jonathan1055: Remove list from .yml dependencies, as...
pfrenssen’s picture

pfrenssen
Location Sofia
commented
Assigned: jonathan1055 » pfrenssen
Issue tags: +SprintWeekend2016

Assigning for review. Working from the global sprint weekend in Sofia :)

jonathan1055’s picture

jonathan1055 commented
StatusFileSize
new API test module notes.pdf24.94 KB

Great, thanks.
My final explanation in #5 is a bit muddled, I think. The new fields seem to be stored in the entity storage (wherever that is) but the actual field names have not been added as columns to the db table node_field_data. This is how the status message gets triggered, I think. I have uploaded my rough notes in case they help.

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Assigned: pfrenssen » Unassigned
Status: Needs review » Needs work

Review:

  1. It's would be a bit easier to review this if the commits were in a separate ticket branch. I now have to review each commit individually. It's not a huge problem but in the future it would be nice if each issue had a separate branch. 
  2.   $types = NodeType::loadMultiple();
  if (!isset($types['scheduler_api_test'])) {
    // Create the node type.
    $node_type = entity_create('node_type', ['type' => 'scheduler_api_test', 'name' => 'Scheduler API testing', 'description' => 'Simulated third-party module used for testing Scheduler API and hook function']);
    $node_type->save();
    // Attach a body field to the node type.
    node_add_body_field($node_type);
  }

    Instead of creating the node type programmatically we can export it as configuration. So the node type can be exported to config/install/node.type.scheduler_api_test.yml, the body field can be exported to config/install/field.field.node.scheduler_api_test.body.yml and the "field_scheduler_test_approved" field can become something like config/install/field.field.node.scheduler_api_test.approved.yml. For examples of this in core take a look at core/profiles/standard.

    It actually looks like the entire hook_install() can be replaced by exported config. Hopefully this will fix the "mismatched entity and/or field definition" error you're seeing.

    The easiest way of doing this by the way is to click together the content type and use the Config Devel module to export the configuration. If you use the machine name 'scheduler_api_test' for the node type you can find all relevant config items with drush cli | grep scheduler_api_test.

    Do we actually need the body field for the test? Haven't looked at them in detail yet.

  3. If you have trouble enabling the test module, try to set up "local development mode" as explained in the header of the file sites/example.settings.local.php. This has an option to allow test modules and themes to be installed.
jonathan1055’s picture

jonathan1055 commented
Title: Convert API testing module to 8.x » API Testing module - conversion to 8.x
Status: Needs work » Needs review
StatusFileSize
new 2655666-10.api_test_module.patch15.92 KB

Yes, I was coming to the conclusion that I could make better progress using the config .yml files, but was trying my best to make it by programming. Got quite a way there but not quite! No, we do not need the body field, it works fine without. Here's a patch which uses exported config files plus the necessary changes to .install, .module and SchedulerApiTestCase.php: Here's a summary of what I have done:

  1. Exported and created five config .yml files, for content type, field storage, field, form display and view display. These allow the module to be installed manually for developing the code and tests
  2. Removed everything from hook_install() as this is all done in the config
  3. Added hook_uninstall() to delete the config items. This should not be necessary but I could not re-install the module without manually deleting so I had to add it.
  4. In .module removed hook_node_info() which is not required
  5. Altered hook_scheduler_allow_publishing() to get the approved field value, and also allow it not to fail for other content types

I my local simpletest SchedulerApiTestCase is now fully green with no fails.

Status: Needs review » Needs work

The last submitted patch, 10: 2655666-10.api_test_module.patch, failed testing.

jonathan1055’s picture

jonathan1055 commented
Status: Needs work » Needs review
StatusFileSize
new 2655666-12.api_test_module.patch20.8 KB

Patch was duff. Somehow my new files were empty. Here's a better one.

Status: Needs review » Needs work

The last submitted patch, 12: 2655666-12.api_test_module.patch, failed testing.

jonathan1055’s picture

jonathan1055 commented
Status: Needs work » Needs review

That's nice, we have 22 passes and no fails :-) and the overall class is green.

2		Scheduler.Drupal\scheduler\Tests\SchedulerApiTestCase
✓		- setUp
✓		- testAllowedPublishing

This test module could be expanded to cover our other hooks, but that's for later. As of now we have a clean set of passes. Setting back to 'needs review' as I'd apreciate any feedback.

jonathan1055’s picture

jonathan1055 commented
StatusFileSize
new 2655666-15.api_test_module.patch20.05 KB

New patch with a few minor changes:

  1. Existing patch no longer applied due to one line commited change in ApiTest, fixed
  2. ApiTest webUser needs permission 'schedule publishing of nodes'. The tests all pass now, but when #2651448: Publish and Unpublish fields are shown for users who do not have the permission is fixed the fields would not be shown
  3. Removed some of my commented-out export code and whitespace errors

Status: Needs review » Needs work

The last submitted patch, 15: 2655666-15.api_test_module.patch, failed testing.

jonathan1055’s picture

jonathan1055 commented
Status: Needs work » Needs review

ApiTest still all green with 22 passes.
Setting back to 'needs review'.

jonathan1055’s picture

jonathan1055 commented

Now that work on #2651338: Create a service for the Scheduler API has started we need this testing module active and running, so the hooks can be tested. As it is only testing code I am going to commit the changes in #15 now. Happy to make further adjustments if anyone wants to do a review later.

  • jonathan1055 committed 25c7c10 on 8.x-1.x 
    Issue #2655666 by jonathan1055: part 2 - five config files for API...
  • jonathan1055 committed 5e65751 on 8.x-1.x
    Issue #2655666 by jonathan1055: part 1 - new .module and .install for...
  • jonathan1055 committed e3e5c52 on 8.x-1.x
    Issue #2655666 by jonathan1055: part 3 - test cases for API Testing...
jonathan1055’s picture

jonathan1055 commented

Good.

2		Scheduler.Drupal\scheduler\Tests\SchedulerApiTestCase
✓		- setUp
✓		- testAllowedPublishing

Overall we have 24 classes pass, 3 fails.

Leaving this issue open for now, as we should expand the API test module to cover more hooks than than just hook_scheduler_allow()

jonathan1055’s picture

jonathan1055 commented
Assigned: Unassigned » jonathan1055
StatusFileSize
new 2655666-21.api_test_module.patch16.6 KB

Added new field to test approved unpublishing (and in doing so, renamed the existing field for clarity). Added test coverage for hook_scheduler_allow_unpublishing(), and added dummy functions for the remainder of the hooks, to be completed in subsequent patches (this one is big enough already)

  • jonathan1055 committed a4ff462 on 8.x-1.x 
    Issue #2655666 by jonathan1055: API Testing - expanded coverage and new...
jonathan1055’s picture

jonathan1055 commented
Issue tags: -SprintWeekend2016
StatusFileSize
new 2655666-23.api_test_rest_of_hooks.patch18.23 KB

I have now added test coverage for all the other API hooks: hook_scheduler_api(), hook_scheduler_nid_list() and hook_scheduler_nid_list_alter()

The changes at the top of the patch are just to rename two variables, as I realised they clashed with $this->nodetype which was needed in the new tests.

  • jonathan1055 committed 42def9e on 8.x-1.x 
    Issue #2655666 by jonathan1055: Added test coverage for the rest of the...
jonathan1055’s picture

jonathan1055 commented

We now have three extra test classes and all the API hooks are covered. There is one scenario (enter past date, 'publish immediately') which is not tested yet, but the other four $actions are covered.

Good to get this in so that work on #2651338: Create a service for the Scheduler API can progress.

pfrenssen’s picture

pfrenssen
Location Sofia
commented 
  1. +++ b/src/Tests/SchedulerApiTestCase.php
@@ -31,22 +31,22 @@ class SchedulerApiTestCase extends SchedulerTestBase {
+    if ($this->custom_nodetype) {
+      $this->pass('Custom node type ' . $this->custom_name . ' "' . $this->custom_nodetype->get('name') . '"  created during install');
       // Do not need to enable this node type for scheduler as that is already
       // pre-configured in node.type.scheduler_api_test.yml
     }
     else {
-      $this->fail('*** Custom node type ' . $this->custom_type . ' does not exist. Testing abandoned ***');
+      $this->fail('*** Custom node type ' . $this->custom_name . ' does not exist. Testing abandoned ***');
       return;
     }

    You can use an assert instead of doing an if statement here. You can do for example:

    $this->assertNotNull($this->nodeType);

  2. +++ b/src/Tests/SchedulerApiTestCase.php
@@ -56,25 +56,25 @@ class SchedulerApiTestCase extends SchedulerTestBase {
-   * Tests hook_scheduler_allow().
+   * @covers hook_scheduler_allow_publishing() and hook_scheduler_allow_unpublishing()

    @covers is a tag that is used by code coverage checks by PHPUnit. I don't think this works for procedural code, and it's not used in tests that extend WebTestBase since they based on Simpletest rather than PHPUnit. Simpletest doesn't read these annotations.

    I don't mind keeping it though since the intention is clear for human readers. 

  3. +++ b/src/Tests/SchedulerApiTestCase.php
@@ -56,25 +56,25 @@ class SchedulerApiTestCase extends SchedulerTestBase {
-    if (empty($this->nodetype)) {
-      $this->fail('*** Custom node type ' . $this->custom_type . ' does not exist. Testing abandoned ***');
+    if (empty($this->custom_nodetype)) {
+      $this->fail('*** Custom node type ' . $this->custom_name . ' does not exist. Testing abandoned ***');
       return;
     }

    Do an assert here if you need to test that the node type exists.

    It's a bit strange to start the test immediately with an assert. Usually you start by performing an action, and then check if the expected result has been achieved. Is there any reason why you think this node type wouldn't be created? Or is this perhaps to help debugging while developing this? 

  4. +++ b/src/Tests/SchedulerApiTestCase.php
@@ -96,7 +96,7 @@ class SchedulerApiTestCase extends SchedulerTestBase {
-    $this->nodetype->setThirdPartySetting('scheduler', 'publish_past_date', 'publish')->save();
+    $this->custom_nodetype->setThirdPartySetting('scheduler', 'publish_past_date', 'publish')->save();

    We should use camelCase for $this->custom_nodetype.

jonathan1055’s picture

jonathan1055 commented
StatusFileSize
new 2655666-27.api_test_changes_after_review.patch4.86 KB

Thanks for the review

  1. I added the two IF tests to halt the entire run if the content type was not loaded. However, this originated when the nodetype was being created by code in hook_install and during development it was annoying and time-wasting for all the tests to run and produce multiple exceptions when the content type failed. Now that I have redesigned it to work from config .yml and it now runs I will change it to a single assertion and not bother with the ' else return' part.
  2. I think if @covers is not actually used in simpletests then we should not use it. Just causes confusion. I have changed it to plain text
  3. Yes this came about during development, and when the .install file was doing processing (see 1 above).
  4. I will change those two properties to camelCase

Patch 27 attached to address all these issues. I have also completed the final piece of the test to cover 'publish immediately' but will keep that for a separate patch not to get mixed in with these review fixes.

jonathan1055’s picture

jonathan1055 commented

Just for information sharing, I found an even better way to allow the test module to be used as a real module which can be manually enabled and run during development, but without duplicating any code files. In a unix-based environment I created a symbolic link called scheduler_api_test in the real modules folder, which points to the source test file folder. 

cd /path_to_your_drupal8/modules
ln -s /path_to_your_drupal8/modules/scheduler/tests/modules/ scheduler_api_test
ls -l

This means you can enable the module for manual testing and development, make code changes to the files and not have any duplication and/or copying of code.

jonathan1055’s picture

jonathan1055 commented
StatusFileSize
new 2655666-29.api_test_publish_immediately.patch2.53 KB

Here are the final changes to cover hook_scheduler_api when publishing immediately. This patch is in addition to #27, but they are independent and do not clash, so can be applied in either order.

jonathan1055’s picture

jonathan1055 commented

Good. For SchedulerApiTestCase before we had 93 passes, now we get 96 passes.
Pieter, when you are happy with my changes from the review I will commit these two, and then this issue can be closed. One more blocker complete.

  • jonathan1055 committed 0e94d9c on 8.x-1.x 
    Issue #2655666 by jonathan1055: API Testing changes after review in 26...

  • jonathan1055 committed 134d035 on 8.x-1.x 
    Issue #2655666 by jonathan1055: API Testing - final coverage from patch...

  • jonathan1055 committed 47cd6f1 on 8.x-1.x 
    Issue #2655666 by jonathan1055: API Testing - stronger tests for allowed...
jonathan1055’s picture

jonathan1055 commented
Assigned: jonathan1055 » Unassigned
Status: Needs review » Fixed

I have committed the changes following review in #26/#27, the final test coverage as per #29, and also a strengthening of a couple of the assertions in testAllowedPublishingAndUnpublishing() which were giving mis-leading positive results if a previous assertion failed.

This is now fully completed (as far as I know) and I am marking it fixed :-)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

jonathan1055’s picture

jonathan1055 commented
Status: Closed (fixed) » Needs review
StatusFileSize
new 2655666-36.avoid_side-effects_on_non_test_content.patch6.58 KB

Small change to insulate non-test content for unwanted changes when then API Test module is enabled interactively (as described in #28). The text 'API TEST' is added to the node title, and checked in the api module. Ordinary nodes without this are therefore unaffected during interactive development.

  • jonathan1055 committed 5223fe1 on 8.x-1.x 
    Issue #2655666 by jonathan1055: Avoid side-effects on non-API Test...
jonathan1055’s picture

jonathan1055 commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

jonathan1055’s picture

jonathan1055 commented

We now have a second test module, which I put in a new folder tests/modules/scheduler_access_test/ - see #2700209: Disable node access checks during cron publish/unpublish

So for consistency I have moved the API test module files from tests/modules/ into the properly segregated tests/modules/scheduler_api_test/ folder. The automatic commit comment did not get written (maybe because this issue was closed?) so here is the link
http://cgit.drupalcode.org/scheduler/commit/?id=369b0d6

jonathan1055’s picture

jonathan1055 commented

Commted some changes to make it cleaner to delete all config items automatically on uninstalling this module.