Automated testing in 8.x [meta]

Hi Pieter,
You probably know this already, but it looks like we need to convert scheduler_test.info to scheduler_test.info.yml. The test log appears ok down to a point

04:31:04 cd /var/www/html && sudo -u www-data php /var/www/html/core/scripts/run-tests.sh  --url http://localhost/checkout --dburl mysql://drupaltestbot:drupaltestbotpw@drupaltestbot-db-mysql-5-5/jenkins_default_29129  --color --keep-results --color --concurrency 31 --sqlite /var/www/html/results/simpletest.sqlite --php /opt/phpenv/shims/php --directory modules/scheduler
04:31:04 Command created as exec id 1969e93f
*** all seems to match OK above here ***

04:31:04   ERROR: No valid tests were specified.
04:31:05 HTTP/1.1 200 OK
04:31:05 Content-Type: application/vnd.docker.raw-stream

I've only just now taken my first look at D8 code and testing, so please excuse me while I learn and catch up, and apologies if I say things you are already aware of.

Jonathan

Now, I think I am wrong. We only need a .info.yml file if the tests are contained in a separate 'testing' module. I will keep investigating.

Pieter,
Are your commits above included in the latest -dev release from 14th Oct. I started looking at the test files but it seems not all of the changes are there, for example splitting the classes into different files.

No hurry for a reply as I won't be able to work on this over the weekend.

Jonathan

Ah, I see that you made the commits in #2 on 16th October but the latest 8.x dev release on our project page is 14th October. I have not been keeping up with how you are running things - is there more than one branch on 8.x and your test changes are not available for download yet?

Just wanted to point to you guys that this test is blocking any other ticket as the tests are failing and the issue status is automatically changed to "Needs review" (as commits in drupal 7 are going after checking through 8.x version).
e.g: https://www.drupal.org/node/2597233

Thanks hugronaphor. Yes, this is now a high priority I think.
By the way, did you know that you can refer to an issue using [# 2597233] to give #2597233: The FALSE value for #group FAPI property does not have expected behaviour which shows the title and status.

Pieter, you have plenty to do, and I am just starting on the 8.x issues, so I have assigned this to me. I need to work out how to work on that branch, not the head, but that's all part of the great git learning.

I've applied the patches to remove variable_set and variable_get from the new test files in branch 2594615. The test show up in the Simpletest list and they start running. However, I get a fatal error:

Drupal\Component\Plugin\Exception\PluginNotFoundException: The "node_type" entity type does not exist. in Drupal\Core\Entity\EntityManager->getDefinition() (line 261 of /Library/WebServer/Documents/drupal8/core/lib/Drupal/Core/Entity/EntityManager.php). 

The above is for SchedulerDateCombinedFunctionalTest.php in line 56, but I also get the same error in SchedulerFunctionalTest.php.

Any suggestions or ideas would be welcome. I may commit the changes I've made so far, on this branch, as they are definitely required, even though the tests don't work yet. Is that OK?

@jonathan1055 - It seems like you are not enabling the node module, that's why this error is showing up.

Thanks Joshi. Actually, it appears that I was not enabling any modules :-) I've just taken over this task from Pieter and I had not checked in detail how far he'd got on https://www.drupal.org/node/2166895 The files have been moved, split up and renamed, but the modules are no longer enabled by

  public function setUp() {
    parent::setUp('scheduler');

but it is now

  public static $modules = array('scheduler');

In D7 we did not explicitly enable the node module, but enabling 'node' in this way does remove the error. Thanks!
Attempting to enable scheduler gives errors in views.view.scheduled_content:display but atleast we are now making progress.

I had managed finally to remove all the fatal errors which prevented the tests from running, and got the tests to finish! (not all passes, but that's a separate issue). It involved:

• Repeating the $config changes from #2418527: Replace variable_set() with config
• adding definition of $config in the various test functions and replacing variable_set
• some $config->set calls fail with fatal error 'cannot set immutable' when using $config = \Drupal::config('scheduler.settings') but seems to be OK using $config = $this->config('scheduler.settings')
• adding public static $modules = array('node', 'scheduler')
• removing getInfo() and replacing the data with phpDoc-style lines
• the files need to be unix LF, not windows CRLF, for the correct parsing of phpDoc data https://www.drupal.org/node/2301125
• There is a fatal error processing config/install/views.view.scheduler_scheduled_content.yml so I commented out all the lines, to allow the tests to run. I will follow up this on issue #2423449: Convert the overview of scheduled nodes into a view
• function ->randomName() no longer exists, replaced calls with ->randomString(15)
• tidied up the @file info, which did have all three classes in each file.

I had wondered if the views problem was because I was on D8 beta-11 so decided to update to the latest rc-3. I did not know that this would require updating my PHP from 5.4 to 5.5, so that add a bit of delay. Then the issue of !message placeholders being invalid caused more trouble. At the moment I cannot run tests because the existing devel quite has ! errors, but the latest devel causes WSOD. I have never had a smooth D8 upgrade, it is always heart-bleedingly tedious with poor messages (or none) and lots of effort expended. I hope that now we are on RC3 it will be slightly more stable.

I am considering merging this work into the main 8.x so that we can see how D.O. tests cope with the changes. Many other issues will be easier to follow when we get testing running, so I do not think it need to be in a separate branch anymore, particularly as that involves re-applying patches and commits that have been done to master.

What does everyone else think about this?

I've now realised I do not need to use Devel for testing (of course it is in core). I think I forgot because I saw that 'Testing' is in the 'Development' block on the main configuration page. Thanks for the hint about development mode, although I think I had many of those settings set anyway.

With my code changes so far:

• SchedulerFunctionalTest runs (but takes a long time, 3 minutes mostly) and we have 121 passes, 89 fails, 23 exceptions, 33 debug messages.
• SchedulerDateCombinedFunctionalTest starts but seems to get into a loop. If you refresh the 'running tests' page after a few minutes you get a link to the results, and it has attempted the tests many times over. On doing 'clean environment' you have thousands of tests and hundreds of tables cleaned up
• SchedulerApiTestCase fails immediately because scheduler_test module does not exist (we have not converted it yet)

I wil push the work to this branch, and try to do it in several commits, so that the changes can be noted. Some will be repeats of fixes done on the master branch, but I guess we can sort that out when we come to merge the branches.

I'm not sure if the DrupalCI test bots already test D8 code for contrib projects at the moment.

I think they do, as I have seen it for other modules, but probably not for branches, only for the master. That's why it would be nice to get this branch merged in sometime soon.

ArgHHHH!!! I tried to push my commits to this branch, but I think I have succeeded only in deleting the branch.
I used

git push -u origin :2594615

which is wrong (I now know) and has deleted the branch. I don't know if it had deleted it on my copy only (I hope) or the remote one aswell.

I think I have deleted the remote branch, as trying to view the commit links by Pieter in #2 above give a blank. But my local files and changes are all intact, and it seems I still have branch 2594615 in my local repository. So the work is not lost. I added and committed changes to four files out of the nine, and I'm not sure where those changes have been stored. Any help would be very much appreciated in working out what to do next. Can we re-create the branch in remote repository, then I push these changes to it? (properly this time) and also re-do the changes made in #2.

Phew! Git is great. I've recovered the branch and it is showing on http://cgit.drupalcode.org/scheduler and the earlier commits have reappeared above, plus my new one. There will be a few more to follow.

Hi Pieter,
Good that they run for you now. Pleased I have finally made a contribution to 8.x development :-)

I made a couple of commits on my branch this morning, but had no internet connection (train wifi was not running). Are you ok if I push them to the remote branch? I don't want to clash with any progress you have made. They are needed to get the tests running via the actual D8 core testing interface.

I have not tried the script version yet.

I figured the above would be ok to push, so you can see what I needed to change. If you don't like any of it, that's fine, we can modify or fix it.

I also have the same fix which is needed in #2612326: Change ! placeholders in formatted messages. When we come to merge this branch into Master branch, I presume it wont make it harder if some of the changes are already done? I need the ! change to allow the tests to be run from within the site.

I have tried your tip about running via the script, but I get syntax errors, and I think that when I initiate php via the command line I am getting php 5.4 not 5.5 which my site is using. I'm not sure how to get them aligned.

It seems like that this issue is kind of blocker with very high priority. It also seems like that tests are now running but failing. So only needs to fix the failing tests. So i think it will be great if we go for this issue same as we go for others (mean patches) so that we have more eyes. It is because most of the time when people thinking to review something, they don't like cloning repo and then see the code.

Great work Pieter, and thanks for the explanations.
I think what joshi.rohit100 was implying was that if this branch was merged back into the master branch, then (a) we could all see the current state of the tests without using the branch (b) we could see the results on D.O. (c) more eyes would be able to contribute to fixing the tests.

I have only just learned about branching and stashing and I find it very easy to work with, and quite like it. However, for this most important issue we are starting to get duplicates fixes e.g. #2418527: Replace variable_set() with config #2418523: Replace variable_get() with config and #2612326: Change ! placeholders in formatted messages which have to be fixed in both branches. I'm OK with doing that, but if others would see benefit in merging this branch in now, than that's ok too.

What do you think?

I am done now, just did those two small commits, and wanted to say I'd finished, but the train wifi stopped working again. Yes, OK you can reverse the view, and then we can work out why it causes the fatal error. Just for the record, here are the results so far:

SchedulerFunctionalTest

Back at #23 we had:

100 passes, 19 fails, 64 exceptions, 17 debug messages

Now that form posting is fixed we have many more attempts, and hence more fails:

121 passes, 89 fails, 16 exceptions, 33 debug messages

but when I run via the run-tests.script I get slightly different numbers - two passes are now fails and there is a huge increase in exceptions:

119 passes  91 fails 1091 exceptions

SchedulerApiTestCase

Because the module .info is not yet converted to .yml the tests do not get started:

0 passes, 2 fails, 1 exception via site
0 passes, 1 fails, 1 exceptions via script

SchedulerDateCombinedFunctionalTest

19 passes, 7 fails, 19 exceptions, 2 debug messages via site
19 passes, 7 fails, 232 exceptions via script

The full test run takes 4+ minutes which seems slower than expected, compared to D7.

Reassigned as requested.
I have found some more places that need simple changes for D8 syntax, and these mean more tests run and we get more passes. I don't have the precise code details with me at the moment but from memory:

• In the date tests several drupalPost() calls now need to be drupalPostForm()
• When calling the admin config form, the keys of the settings parameters array should not have 'scheduler_' at the start. This removes the 'cannot find field' errors
• The node/add form 'save' button is not working because the text has changed. It is now a selection, and the text of the default choice is 'save and publish'

I suggest we wait until you have done the rebasing and merging, before making any more patches/commits.

This is starting to look good. We now get tests running on D.O. (even though there are many failures).

Did you intend to make your three new commits to the branch? I thought that after you had merged we would be dropping the branch and then continue making the next changes directly in master, so that everyone gets the improvements immediately. Or maybe you had an alternative plan?

I can progress my next fixes locally anyway, and they can be applied later depending on your response to the above.

OK. that's fine. I will push a few more commits to this branch - I am finding lots of things to fix, and the failure numbers are coming down :-)

One the fails is

    // Verify that the scheduler table is not empty.
    $this->assertTrue(db_query_range('SELECT 1 FROM {scheduler}', 0, 1)->fetchField(), 'Scheduler table is not empty');

The scheduler table does not exist in D8. Should we go to the node_field_data table instead? Then check for any publish_on values, or specifically for a value for the node just created. Is that worth a test? It's kind of double checking that the core field functionality has worked, but I'm ok with doing that, providing it will give a consistent useful result.

The commit in #46 matches the main branch commit in #2418523: Replace variable_get() with config #30 which is needed for the tests to run.

Commit in #49 matches #2621122: Use ->value for $node->unpublish_on which is vital for the tests to run without fatal errors.

I've been working on SchedulerFunctionalTest.php and schedulerTestBase.php, and have now got all the tests to run without any fatal errors. The breakdown of results at the moment is:

1. testScheduler() 42 passes, 5 fails, 9 exceptions
2. testSchedulerPastDates() 30 passes, 2 fails, 1 exception
3. testRevisioning() 7 passes, 4 fails, 0 exceptions
4. testExtraFields() 18 passes, 3 fails, 1 exception
5. testRequiredScheduling() 53 passes, 7 fails, 1 exception
6. testValidationDuringEdit() 14 passes, 1 fail, 1 exception
7. testScheduledNodeDelete() 16 passes, 4 fails, 1 exception
8. testScheduledNodeDelete() 16 passes, 0 fails, 1 exception

Overall 180 passes (up from 121 from last update), 22 fails (down from 89)
Total of 202 tests and 14 exceptions

Quick summary of changes to schedulerTestBase.php

• Fixed D8 date array keys
• removed select of scheduler table
• changed update to use node_field_data
• change node_revision field from log to revision_log

Changes to SchedulerFunctionalTest.php

• use Drupal\node\Entity\NodeType
• change lots of incorrect config->set to setThirdPartySetting
• Randomstring instead of fixed title text
• change many publish_on and unpublish_on to use new split date and time values
• Change 'save' key in edit form to 'save and publish', 'save and keep published' etc.

I have pushed the two changed files to this branch. The code includes several @TODO with questions, remarks and debug messages on the parts which still need attention.

One of the problems hindering progress is the default timezone warning which also stops some of the pages being rendered. The warning was

Exception: DateTime::__construct():
It is not safe to rely on the system's timezone settings. You are *required* to use the date.timezone setting or the date_default_timezone_set() function. In case you used any of those methods and you are still getting this warning, you most likely misspeled the timezone identifier. We selected the timezone 'UTC' for now, but please set date.timezone to select your timezone. in DateTime->__construct()
(line 215 of core/lib/Drupal/Core/Datetime/DateFormatter.php). 

Using date_default_timezone_set() did not remove the message, so I have now added a date.timezone setting to my php.ini file. This removed the warnings and allows the pages to be rendered properly.

Then the assertText checks had a chance of matching and I could see that the nodes were published and unpublished correctly but the randomString text used for detection was causing the jQuery not to match. Changing this to the more simpler randomMachineName() allowed the text to be found. We now have a full set of passes in the first test testScheduler() which is the first time we get all green.

Overall we now have results of
185 passes, 17 fails, 0 exceptions

Added tests to cover #2625572: Admin setting 'Enable content type for scheduler' is ignored

Overall for SchedulerFunctionalTest.php we now have:

total 214 passes, 24 fails, 0 exceptions

OK. I've been doing some investigation into testRevisioning and am part way through, so maybe if you want to start on any of the others, but not that one. I wouldn't want you to use your time repeating what I've done. I have not made any changes to any other tests since the push at #55.

Hey, I consider that you are doing all the clever work!

Thanks for the pushes. I was going to update this issue summary with the current state of tests and link to the separate issues that will fix them. Although in my work in testRevisioning it seems like it might be the fault of the test files not the main functions. I think I may need your expert assistance on object-oriented code and namespaces, entities, etc. I'll get some details up here tomorrow.

Basic conversion of scheduler_test module .info to .info.yml
Just wanted to get this change out of the way, but I am not working on those tests yet.

When working on testRevisioning I realised that revisions during cron work ok for real, but not in the tests.
Within testRevisioning() we have:

    // Enable revisioning.
    $entity = $node->type->entity;
    $entity->setThirdPartySetting('scheduler', 'publish_revision', TRUE);
    $entity->setThirdPartySetting('scheduler', 'unpublish_revision', TRUE);

and this works OK, at least within this function. The value is set to TRUE if retrieved immediately here. But in _scheduler_publish() the retreived config value for 'publish_revision' is FALSE. If the value is forced to TRUE then a revision is created during the tests, so that part is working correctly. It is just the setting and retrieval of the thirdPartySetting which is not working. In fact none of the third party settings that are applied in the test files are available in the main functions. The values applied in setUp were not even available in the test cases

$entity in testRevisioning():
Drupal\node\Entity\NodeType Object
(
    [type:protected] => page
    [name:protected] => Basic page
    [description:protected] => 
    [help:protected] => 
    [new_revision:protected] => 
    [preview_mode:protected] => 1
    [display_submitted:protected] => 1
    [originalId:protected] => page
    [pluginConfigKey:protected] => 
    [status:protected] => 1
    [uuid:protected] => f16a3398-6fd5-481f-8a09-9c5d0f697806
    [isSyncing:Drupal\Core\Config\Entity\ConfigEntityBase:private] => 
    [isUninstalling:Drupal\Core\Config\Entity\ConfigEntityBase:private] => 
    [langcode:protected] => en
    [third_party_settings:protected] => Array
        (
            [scheduler] => Array
                (
                    [publish_revision] => 1
                    [unpublish_revision] => 1
                )

        )

    [trustedData:protected] => 
    [entityTypeId:protected] => node_type
    [enforceIsNew:protected] => 

The array for [third_party_settings:protected] should have more here. Then I discovered it is not sufficient just to call ->setThirdPartySetting, you also need ->save() for the values to be stored permanently. After I added the ->save() it produced lots of schema errors in the tests, which halt the test run:

Drupal\Core\Config\Schema\SchemaIncompleteException:
Schema errors for node.type.page with the following errors:
  node.type.page:third_party_settings.scheduler.publish_revision missing schema,
in Drupal\Core\Config\Testing\ConfigSchemaChecker->onConfigSave()
(line 98 of /Library/WebServer/Documents/drupal8/core/lib/Drupal/Core/Config/Testing/ConfigSchemaChecker.php).

I tried several variations within scheduler.schema.yml and eventually discovered that instead of
node_type.third_party.scheduler:
it should be
node.type.*.third_party.scheduler:

I found that the config inspector module is extremely helpful https://www.drupal.org/project/config_inspector

Summary of changes in the three commits above:

SchedulerFunctionalTest.php

scheduler.schema.yml

changed to node.type.*.third_party.scheduler
added labels for all Scheduler main settings
added missing 'expand_fieldset' content-type setting

SchedulerTestBase.php

Remove debug
Remove unnecessary field from db_select
No material differences.

We should test the schema changes in the main branch and get that change applied there too. It seems that previously the settings were being stored from the content type form, even though the schema was not structured correctly. The testing checks the schema whereas using the real site does not.

Total 221 passes, 19 fails, 0 exceptions

All of the above gave us 5 more passes! (plus 2 new tests which pass). Yes, only 5 were fixed (4 in revisioning and 1 in pastDates) but it means that the settings are now being stored and made available in the real code, and without that we'd have no hope of correcting the remaining tests. I may change the summary of this issue to give a list of the tests status and links to the issues where the code can be corrected.

Here's the patch from the schema.yml changes if anyone would like to apply it and do further testing. The content-type specific settings seem to be working fine. If you use the Config Inspector module, you will probably see duplicate items, but after a clear cache and resave of the settings it seems to get sorted out. I noticed a problem with the content-type form (before this change) where the boolean values of TRUE get the correct radio button lit and showing, but if the stored setting is FALSE we get neither radio button lit. This needs a separate issue though.

For now we just need to check that my schema changes are OK. Certainly they are fine with all the automated tests so far, but I am aware that we need to do real testing too.

#68 fixes testExtraFields() and this test is all green now. Needed slight changes to the xpath values, as classes and ids are different. Also the class 'collapsed' is no longer used, but a new value 'open' is present.

Out of the eight test classes we have four which fully pass.

I have fixed testRequiredScheduling(), which involved (a) correcting the array key names to get the intended values into nodes before testing the editing, (b) changing the value of publish_on to a timestamp number not formatted text, and (c) a change to the text of the validation message to check for. The rest of the changes are with debug calls.

All of the 60 tests in the class now pass. However there is still work to do because I do not know how to change the default status of a content type before the /node/add/page is called. This affects tests 1/2 and 6/7 so currently 1 & 2 are testing the same situation, as are 6 & 7. I've searched the API pages but so far have not found out how to set the default status before a node is added. I've added an assert(FALSE) which displays a TODO message to make sure we do not forget and let this go by unfixed.

As the main SchedulerFunctionalTest is progressing nicely I thought I would do some work on SchedulerDateCombinedFunctionalTest.php. Here is a summary of the changes in #72:

• use Drupal\node\Entity\NodeType
• change to use setThirdPartySetting()
• use shorthand array notation to match Pieter's changes
• $this->drupalPost to $this->drupalPostForm
• $edit array key names
• form button text

Previously: 19 passes, 7 fails, 19 exceptions
Now: 37 passes, 6 fails and 0 exceptions

The commit in #74 repliactes the commit in #2490584: Validate that unpublish date is entered if publish date is entered
This fixes the one failing test in testValidationDuringEdit. Thanks Joekers.

The merge went without problems. This is ready to be merged back to 8.x-1.x if @jonathan1055 agrees to do this.

That's good news. I thought it would be a good time to try this, as at the moment it is clean. I had to adjust the last commit from the master branch to get it in to this branch, and there are plenty more waiting. Yes it will be good to merge it all back. I've not actually done a full branch merge, and I will not be able to make any changes for another 5 hours. So if you want to do this now, please go ahead. If you do, let me know what git commands you use, and I will do the next one. Or let me know what commands you used last time, and will do this merge tonight.

Thanks for all the great feedback and ideas in #78. I agree with all you've said. Now that you have checked, lets merge before we implement any of these tidy-ups and enhancements. Then I will address those points back on this branch after the merge.

Excellent. Thanks for doing that. Thanks also for the git commands, I will try the next one.

Added the check on $node in helpTestScheduler to avoid a fatal error which stops the tests running. in #2490574: Validate that the publication date is in the future see comment #22 for details.

Update test results after adding the two new tests.
Added links to the corresponding issues.

#92/93 fix #2630836: Default timezone vs User timezone in automated tests

In #95 commited constraints from #2490576: Validate that the unpublish date is in the future to the testing branch, to keep things in line and allow the use of this branch when doing further work on tests. Updated the summary with new totals.

The commit in #97 merged into the test branch the changes from #2490574: Validate that the publication date is in the future and #2490580: Validate that the unpublish date is later than the publish date.

I've been working on the problem of setting the default node status, which is needed in testRequiredScheduling. With the help of examples in core node/src/Tests/NodeFormButtonsTest.php it is now done. When checking the verbose page output you can tell the default node status has been changed because the default button text changes.

I am attaching the patch here, but I think the tests will actually run against 8.x-1.x master. At this time it is fine because the test branch does not have anything extra.

Things are getting better.
✓ - testRequiredScheduling
Overall 253 passes and 7 fails.
11 class passes and 5 fails.

The commit in 102 fixes the problem from #71 above.
Merged into master branch too.

Yes, OK. I agree about the class change.
But I have actually been working on testNonEnabledNodeType() this evening too. [just got back to Drupal after being away over the holidays]. That is the only test which fails in that file, and the test needs a slight change in conjunction with the possible fix in scheduler_form_node_form_alter. Yes, please make the class change, but I don't want you to waste your time duplicating the fixes to the actual assertions that I have got in the pipeline.
Or if you'd like to start on the API tests (I think you wrote them in the first place in 7.x) that would be great too.

or I can just post here the changes to the actual assertions and you can incorporate them when you make the class changes, that would be fine too.

I have moved the tests for non-enabled content type to a new class, as you suggested, in SchedulerNonEnabledTypeTest.php. The fix in #2597233: The FALSE value for #group FAPI property does not have expected behaviour does indeed correct the error of showing the date fields for non-enabled types. However, I have spotted another error with non-enabled types, which I have now added tests for. Because the base fields exist for all content types we should still check in scheduler.cron.inc that the node being processed is actually enabled for scheduler. We allow third-party modules to alter the list of nodes, and anything could set a publish_on date in the db table. We should prevent our cron processing from acting on any nodes which are not enabled.

The attached patch removes the old test from SchedulerFunctionalTest.php and also makes one safety-check in scheduler.cron.inc to prevent cron failures. The new test class should pass for the form field checks but fail on allowing non-enabled types to be processed. Then I'll add a fix and post a second patch.

I noticed that the fieldset tests may not pass anymore, so we might not get a full set of greens for the main test class. See #2645818: Date input fieldset expand/collapse is not working for details.

As expected in #109 we get 6 fails on checkNonEnabledTypes.

Here is a patch which includes a check in scheduler.cron.inc to avoid processing non-enabled types.

That's good, we have one extra class passing now.

✓		- checkNonEnabledTypes

We also now have our first green row in the console output. I think the fix to cron.inc should be committed against #2625572: Admin setting 'Enable content type for scheduler' is ignored as it is a correction to the main code base not the test. The new test class needs tidying up and debug removed, which I will do, then I would appreciate someones OO knowledge for a review, as there may be some things I could do differently.

1. +++ b/src/Tests/SchedulerNonEnabledTypeTest.php
   @@ -0,0 +1,193 @@
   +
   +use Drupal\Component\Utility\SafeMarkup;
   +use Drupal\Core\Entity\EntityStorageInterface;
   +use Drupal\node\Entity\Node;
   +use Drupal\node\Entity\NodeType;
   +use Drupal\node\NodeInterface;
   +use Drupal\node\NodeTypeInterface;
   +use Drupal\simpletest\WebTestBase;
   +
   

   Some unused / un-required use statments.

2. +++ b/src/Tests/SchedulerNonEnabledTypeTest.php
   @@ -0,0 +1,193 @@
   +    $this->content_name = 'not_for_scheduler';
   +    $this->drupalCreateContentType(['type' => $this->content_name, 'name' => t('Not for Scheduler')]);
   

   You can assign it to property so that it can be later used in testNonEnabledContentType, instead of loading there.

3. +++ b/src/Tests/SchedulerNonEnabledTypeTest.php
   @@ -0,0 +1,193 @@
   +    $body = $this->randomMachineName(30);
   +    $edit = [
   +      'title' => $title,
   +      'promote' => 1,
   +      'status' => 0,
   +      'body' => $body,
   +      'type' => $this->content_name,
   +    ];
   +    $node = $this->drupalCreateNode($edit);
   +    db_update('node_field_data')->fields(array('publish_on' => REQUEST_TIME - 2))->condition('nid', $node->id())->execute();
   

   same as above.

4. +++ b/src/Tests/SchedulerNonEnabledTypeTest.php
   @@ -0,0 +1,193 @@
   +    $edit = [
   +      'title' => $title,
   +      'promote' => 1,
   +      'status' => 1,
   +      'body' => $body,
   +      'type' => $this->content_name,
   +    ];
   +    $node = $this->drupalCreateNode($edit);
   +    db_update('node_field_data')->fields(array('unpublish_on' => REQUEST_TIME - 1))->condition('nid', $node->id())->execute();
   

   You can create these two nodes (published / unpublished) in setup so you don't have to create these nodes multiple times in test method as currently.

Hi Joshi,
Thank you for the review of SchedulerNonEnabledTypeTest.php

1. Some unused / un-required use statments.

   I'm not surprised as I was adding many statements in order to try out and develop the code. Which ones are not required?

2. Do you mean the node type property? i.e store the result of drupalCreateContentType. I will try that, thanks

3. same as above.

   what exactly?

4. Not sure that would be any help, as I want clean created nodes, so that the dates can be set fresh. If I was to re-use nodes I'd have to clear the dates via edit or db_update. Is that better than creating them fresh?

Thanks again. I will be good to get this test moved, and also the minor fixes to scheduler.cron.inc then we will have more green test passes.

Here's a re-roll for SchedulerNonEnabledTypeTest. Changes since #111:

1. removed debug
2. removed unnecessary use statements
3. store the result of creating the content type, instead of re-loading it
4. Added sequence number to the text, for easier following of the processing
5. improved comments and tidied up the code

I've attached an interdiff so you can see exactly what is changed. I am going to commit this, but I'm ok if it needs minor tweaks later. There are several more test changes in the pipeline, and getting this done and merged into the main 8.x branch will be good progress.

I've added checks to the 'extraFields' test to check that the fieldset is expanded when publish-on or unpublish-on dates exist in the node being editted. These tests currently fail as the code for this was dropped (probably by accident) on converting from 7.x. See #2645818-8: Date input fieldset expand/collapse is not working for details.

Tests added for user without permission, to cover #2651448: Publish and Unpublish fields are shown for users who do not have the permission

Updated results (and restored some deleted text in the summary) after #2645818: Date input fieldset expand/collapse is not working http://cgit.drupalcode.org/scheduler/commit/?id=d3d0669

I've been working on the default date processing, and this allowed me to fix the date tests. The changes in this patch are:

1. Remove 'node' and 'datetime' from modules required. Both of these are automatically added by 'scheduler' pre-requisites
2. Use $this->seconds = 23400 instead of repeating the hard-coded number
3. Correct the text of the validation messages to search for, as these are slightly different as now produced by Datetime::validateDatetime not by scheduler_node_validate()
4. Change $this->assertRaw to $this->assertText because the messages now contain markup and assertText strips this out
5. Change $this->clickLink(t('Edit')); to $this->drupalGet('node/x/edit'); because the edit link is not necessarily found, and the url is more robust
6. Added @todo about switching to date.formatter
7. Added @todo regarding no longer needing this file as separate, now that datetime functionality is in core not contrib
8. Comment improvements and code tidy-up

This patch may remove one failed test, but the default time functionality is not added yet. I will add that patch to #2644690: Include default time processing in datetime widget for 8.x when these test changes are committed.

+++ b/src/Tests/SchedulerDateCombinedFunctionalTest.php
@@ -102,26 +107,32 @@ class SchedulerDateCombinedFunctionalTest extends SchedulerTestBase {
+    $this->drupalGet('node/' . $node->id() . '/edit');
+    $time_text = format_date(strtotime('tomorrow', REQUEST_TIME) + $this->seconds, 'custom', 'H:i:s');
+    $this->assertFieldByName('publish_on[0][value][time]', $time_text, 'The default time offset has been added to the date field when scheduling content for publication.');

use service (or todo for this)

use service (or todo for this)

Hi Joshi,
Thanks for the feedback but can you be a bit more specific? Instead of pasting chunks of the patch if you put just the actual line of code it would be more obvious. If you mean the date, I already have @todo Use \Drupal::service('date.formatter').

Jonathan

[Also, something must be going on with the test servers, as this patch has been queued for four hours and not started yet.]

My point is that date formatter service should be used for format_date() or there should be a todo for this (I am guessing todo that you have mentioned is only for date() ?)

Yes, I meant it as a generic todo for the whole file, but I note your point, thanks. I will address those things when the tests run cleanly.

Test failed to complete due to calling $node->id() when the node had not saved properly. This is easily avoided:

    // Check that the default time has been added to the scheduler form on edit.
    // Protect in case the node was not created. The tests will fail anyway.
    if ($node = $this->drupalGetNodeByTitle($edit['title[0][value]'])) {
      $this->drupalGet('node/' . $node->id() . '/edit');
    }

I also added 'and format_date()' on the @todo ;-)

Good. As expected we get one fewer fail because the 'edit' link is fixed - internally it is 37 passes and 5 fails. Commited this to testing branch and merged to main 8.x to allow testing on #2644690: Include default time processing in datetime widget for 8.x

Updated results after fixing a problem in the date test, adding more assertions to the permissions test and starting work on API testing module conversion
#2655666: API Testing module - conversion to 8.x

Here are some tests for the lightweight cron. This is just the test part of the patch in #2626480-15: scheduler/cron needs a route controller by polynya. Need to check these tests and commit separately first, so that we see the difference when the route is fixed.

+++ b/src/Tests/SchedulerLightweightCronTest.php
@@ -0,0 +1,82 @@
+   * The modules to be loaded for these tests.
+   */
+  public static $modules = ['scheduler', 'locale'];
+

I noticed that every test class is extending schedulertestbase and also defining scheduler module to be enabled. shouldn't scheduler module to be enabled in parent so no need to rewrite again ?

Hi Joshi,
Yes I noticed that too. Currently schedulerTestBase does not have public static $modules = ['scheduler']; but if that is added and the others are removed it all works just fine. Thanks for the prompt, I will make that commit now, before we introduce any more new test files. The patch in #152 needs to be worked on anyway.
Jonathan

I don't see use of enabling locale in #152. Is any ?

No it is not needed - see my comment on #2626480-18: scheduler/cron needs a route controller
I am going to use that issue for refining this new test file, given that that is where polynya first created the file.

Thanks for the hint about $modules in schedulerTestBase, that's worked nicely :-)

During work on #2625572: Admin setting 'Enable content type for scheduler' is ignored I realised that in SchedulerNonEnabledTypeTest we should delete each node after using it, otherwise the cron processing/exceptions affect the results of subsequent assertions. I also display the dblog after each cron to show what has happened and tidied up the comments.

Updated summary and added details for LightweightCron tests.

I am going to split out the test in the large SchedulerFunctionalTests.php file into separate test files. Now that we have a few new test files, such as the lightweightCron and non-enabled nodetypes tests, it will make it consistent to have the others split. Smaller files in this instance are easier to maintain, and changes to one test patch will be less likely to clash with another. It makes re-testing easier (and hence quicker) when working on one part of the functionality. We will soon be expanding the test coverage so I want to get these files split now, to make that easier.

Also we have the setup function in SchedulerTestBase which was not used, and we had common code repeated in the other classes. This is now all in the testbase setup function.

I've only split out two test functions in this patch, just to see how it goes.

Neede to change API setup from protected to public to match ShedulerTestBase

Last of these commits coming up, then we should have a consistent set of twelve test classes.

The four helper functions in SchedulerTestBase were not actually common functions - each is only used in one test class, so I have moved them into the files that need them - keeping code only where it is needed instead of loading it unnecessarily. (1 into FunctionalTest and 3 into RevisioningTest)

Updated summary to reflect new test cases.

Updated after adding test for #2666832: Publish in the past sets date to 1970-01-01

Updated summary after commits in #2666832: Publish in the past sets date to 1970-01-01 and #2655666: API Testing module - conversion to 8.x

Now that #2644690: Include default time processing in datetime widget for 8.x and #2651448: Publish and Unpublish fields are shown for users who do not have the permission have been committed we only have one failing test - the patch is on #2625572: Admin setting 'Enable content type for scheduler' is ignored for review.

I have also deleted the testing branch 2594615 as it has served it's purpose. All the tests have been running for several months now, and it was just an unnecessary overhead to keep both branhces in line, continually merging every commit from one branch into the other.

I have made some minor improvements to the test files, mainly in response to my testing of #2633870: Revisit defaults for third party settings which allowed me to see where the tests were passing due to luck and assumption, instead of testing the actual piece of code in question. Only minor changes, here is a summary:

1. FieldsDisplayTest - one extra assertion to split the check that a fieldset is shown and then that it is open.
2. FunctionalTest - exit earlier if the node was not created, to avoid showing confusing successes and failures.
3. NonEnabledTest - variable name changes for consistency, message text improvements when checking default values.
4. PastDatesTest - set 'required' to false so that tests do not give confusing results. Extra assertion to check default values, then check 'error' value explicitly not implicitly.
5. RequiredTest - extra test case to check default values, not make assumptions.
6. RevisioningTest - make sure value for publish_past_date does not skew the results. Clarified text of messages.
7. Some classes had redundant setUp() which just called parent::setUp() others had none. Removed the empty ones.

Does not need a review, I am going to commit now, but for the record here is a patch, to show the test results.

#2625572: Admin setting 'Enable content type for scheduler' is ignored is fixed.

Added new tests SchedulerTokenReplaceTest and SchedulerMetaInformationTest

All tests pass :-)

All tests have been passing since 9th March https://www.drupal.org/pift-ci-job/206819
Closing this issue now. Good work.

Good to see this meta closed :)

Unassigning.