Add Services Resources

patch seems empty

Where to add this file?

There was an issue with the patch in #2, the delete function was erroneously still the bean_services_delete() function instead of the fieldable_panels_pane_services_delete() like it should have been. Fixed in new patch.

I believe this needs to have full support for revisions, as suggested by the @todo item in a comment? There are also some minor fixes needed in the comments, and at least one occasion where "Fieldable" is spelled "Fiedlable" ;)

Addressing #6:

• Added support for revisions as suggested by the @todo
• Fixed CS issues on some of the comments.
• Fixed typo on "Fiedlable", searched the rest of the patch and there was only one occurrence of this.

FYI: I'm in the process of adding support to fpp into deploy_addon: #2915142: Add Manage Fieldable Panels Panes page

Anyone interested feel free to have a play / review / collaborate =)

fieldable_panels_panes_services_update() still references 'bean_form', "bean" is mentioned in a few comments, and some of the function docblocks need the @return statement separated from the @params by an empty line. But it seems like it's getting close.

Also, lets add some tests to make sure the functionality works - doesn't have to be overly extensive, just the basics.

Fair enough - here's a test verifying that the services do actually appear at the right place.

Thanks Manuel!

The points about the bean comments and bean_form still stand, and that last one makes me wonder what might be broken.

Previous patch was failing with Invalid permission administer services., not very familiar with how the bot works on d7 sorry (works on my local machine), so this may require a few tries to get the module dependencies right.

While I'm at it, also addressing the bean comments and using the fpp edit form instead of the bean form for the update operation.

Let's see if this is it...

OK I'm blocked... not sure why the bot throws Invalid permission administer services, we are enabling services module which is the one providing the permission... any clues why this is happening?

Ah! It's failing to enable all of the modules. Maybe the problem is because of testbot?

Having been alerted to this issue via #2692407: Test_dependencies are downloaded before applying patches, rather than after I think you will need to commit just your change to .info.yml first, adding the test dependencies.

You may be able to avoid adding the new files[] = tests/fpp.deploy.test until afterwards. Do you actually need this line, if the tests are in the correct folder and named according to standards?

There are no standards in D7 for automatically discovering test classes, so all tests have to specifically listed in the info file.

all tests have to specifically listed in the info file.

Hmmm, well maybe, but I have just checked by deleting the two test files[] lines from a contrib module I maintain. Executing run-tests.sh --list still lists the tests to be run, and run-tests.sh --directory sites/all/modules/my_module runs all the tests for the module.

That's because the test classes are cached in the registry.

.. also the --directory option runs some extra magic whereby it ignores the info file entirely and just scans for files with the extension "test" (see run-tests.sh line 475 onwards).

Yes, you are right, the test file information was cached, sorry. This must have been the cause for both my "sucesses" because the --directory option in run-tests.sh searches for *.php in a /tests/ folder, and the D7 files are *.test, so this also fails to find any test files if they are not listed in the .info file. Sorry to distract the issue, back on task ...

While we figure out the testing part of this, I've gone ahead and started testing this manually, the patch is indeed not ready IMHO. Attached patch at least gets the callbacks getting called, and another fix to what looks to be a residual bug form starting off from the bean services integration.

• The index resource seems to work fine with this patch.
• All other resources I am getting access denied, which tells me probably the access callback needs to be looked at.

I am not terribly familiar with fieldable panels panes array of permissions, but I'm guessing if we had global permissions like this, it would simplify things:

• update fieldable panels panes
• create fieldable panels panes
• delete fieldable panels panes

If anyone has a better suggestion I'm all ears :)

OK, I decided to go TDD on this, and happy to say I made a lot of progress. Debugged every resource, they are all working now, and we have full test coverage for all the services resources (passes locally but the bot will complain because of #2692407: Test_dependencies are downloaded before applying patches, rather than after).

Sorry for the big interdiff, also fixed a lot of CS along the way...

Forgot to mention that we now just test services integration, so no need for test dependencies on deploy_addons etc.

The tests give a small error:

PHP Fatal error: Class 'ServicesWebTestCase' not found in fpp.services.test on line 11

I think this is back to the problem of needing to add a composer.json with the dependencies (#2916627: Add a composer.json file to document the dependencies) so that we can then indicate services is now needed.

Yeah tests fail because the bot can't detect there is a new testing dependancy. Submitted a patch to #2916627: Add a composer.json file to document the dependencies :)

Postponing on #2916627: Add a composer.json file to document the dependencies

Blocker is now in, let's see if the tests run properly now :)

Let's actually make use of the new composer.json file..

OK this is very strange, test console output throws error:
Call to undefined function services_endpoint_save() in <em class="placeholder">FppServicesTest-&gt;saveNewEndpoint()</em> (line <em class="placeholder">100</em> of <em class="placeholder">/var/www/html/sites/all/modules/fieldable_panels_panes/tests/fpp.services.test</em>)

But that function is defined in services.module, so that means that the services module is not enabled? I can see the module being pulled in using composer on the log, but only enabling simpletest. It should though, because we are listing it on our info file as a test dependency no?

I'm not sure, it's confounding.

I wonder if the new dependency was committed to fieldable_panels_panes.info first? Might have to do that old trick.

BTW could you please update setUp() to follow the structure used in FPP's other tests? Thanks.

Just cleaning up the test.

I wonder if the new dependency was committed to fieldable_panels_panes.info first? Might have to do that old trick.

I'll submit a new patch to get the dependency on the info file first.

Created #3029146: Add test dependency for services module integration

Now that #3029146 has been committed, this needs a slight reroll.

Reroll of #39

I fixed the branch tests in #3036702: Fix tests for 7.x-1.x branch, so let's see how this patch works now.

Nice, thanks for the quick fix there @DamienMcKenna 🤞

The tests fail because:

Error: Call to undefined function services_endpoint_save() in FppServicesTest->saveNewEndpoint() (line 96 of fpp.services.test).

There was a new version of the Services module out this week, maybe there was an API change?

That function is still there https://cgit.drupalcode.org/services/tree/services.module?h=7.x-3.x#n447

It's the same error we were getting before #3029146: Add test dependency for services module integration got in... seems like the services module is still not being enabled for the test for some reason...

Oh yeah, services needs to be added to the setUp() method.

I thought that ServicesWebTestCase::setup() would enable it but well, let's give that a try.

Sorry, I missed the fact that it was extending ServicesWebTestCase instead of FppTestHelper..

Minor adjustments, changed to using assertEqual() instead assertTrue() for some of the assertions.

Lets get this in the next release.

Per a comment from dww in slack, there's something weird with the build system where it might take "up to 24 hours" for new test dependency to take effect, so I'll try again tomorrow.

+++ b/tests/fpp.services.test
@@ -0,0 +1,310 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function setUp(array $modules = array()) {
+    $modules[] = 'fieldable_panels_panes';
+    parent::setUp($modules);
+
+    $this->endpoint = $this->saveNewEndpoint();
+  }

Try explicitly enabling services here and see if that helps.

For posterity:
https://www.drupal.org/docs/8/creating-custom-modules/let-drupal-8-know-...

test_dependencies - A list of other modules (in the same format as dependencies) that are needed to run certain automated tests for your module on Drupal's automated test runner ("DrupalCI"), but not needed as module dependencies in general (or that are in development as module dependencies but not finalized yet). Note that you need to have the test_dependencies change committed to your Git repository before you try to run a test that depends on it -- you cannot just put the info.yml change into the same patch as the new test. As an alternative, you can also use Composer for test dependences -- see https://www.drupal.org/docs/develop/using-composer/managing-dependencies... for more information.

(emphasis added).

Point is, if you have to wait, you first have to push a commit for test_dependencies into the .info file and then wait. ;)
-- sorry, read the backscroll and I now see #3029146: Add test dependency for services module integration.

Thanks @dww for taking a look at this, was about to loose hope here :D

Re #56

Try explicitly enabling services here and see if that helps.

We already tried that on #49, but it's actually not necessary since this test extends ServicesWebTestCase which sets that up for us :)

Ok, I ran the tests again, theoretically plenty of time for any cached things to be cleared, and it made no difference, the test still fails.

Sorry, I've got no further ideas. The testbots have never been my area. I just saw a question in Slack and tried to help. I think this will require @mixologic to sort it out. :(

Thanks for trying, @dww.

Hrm, but #49 was before #3029146: Add test dependency for services module integration right? I tried re-queuing #49 now that #3029146 is done for over 24 hours, just to see what happens. ;)

Meanwhile, some other possible thoughts:

A) +test_dependencies[] = services

You might need to specify you want the 7.x-3.x branch. I think it should be smart enough to use the recommended branch, but it doesn't hurt to be explicit. This supports module dependencies:

test_dependencies:
  - duration_field:duration_field (>=8.x-2.0-rc2)

(well, that's D8 syntax, not sure about D7).

B) The branch test for services 7.x-3.x-dev is currently failing:

https://www.drupal.org/pift-ci-job/1213343

You might be doomed until you help them upstream get their testing world in order.

C) Services itself depends on ctools. I have no idea if our test bot is smart enough to get that right. Perhaps you should add "ctools" to your test_dependencies, too, just in case.

Thanks again dww.

Per the console output it's downloading Services 7.x-3.23, so that part should be fine.

CTools is listed as a main dependency of the module, so it shouldn't need to be listed as a test dependency too.

The weird part is that the services tests work fine locally for both of us, which wouldn't be the case if it was something in the Services tests, so it seems like something odd with drupalci. BICBW.

Let's add a dependencies item to getInfo(), and some other minor tweaks.

Let's see if manually enabling services fixes the problem.

Still the same error.

Thanks @DamienMcKenna for trying to figure this one out... I'm out of ideas here, it's amazing it still throwing the same error :-s

Minor tweaks.

@mixologic worked out the problem - rest_service requires the libraries module. We'll probably have to do another commit to add it as a test_dependencies line first.

I'm adding the dependency in #3037423: Add libraries as a test dependency for #2163581, because it isn't actually running the new Services test in #73. This will need a reroll after #3037423 is committed.

Rerolled.

The tests are now *running*, but it's showing a bunch of errors.

I think I accidentally removed a line from setUp().

IT'S GREEN!
:-D

whohooo!! :partyparrot:

Committed! Woohoo! Thanks everyone for all your work on this!

\o/

Glad it worked! Yay for sorting it out.

p.s. I still don't understand d.o's issue "credit" system. The commit mentioned me, but this issue doesn't show up in the list at https://www.drupal.org/u/dww and the checkbox doesn't appear to be checked here. Are maintainers expected to manually keep these 2 in sync? *shrug*

@dww: That might have been my mistake when I was submitting the form, sorry. Does it show now?

Indeed it does, thanks!

I don't really care, I'm mostly trying to understand how it (doesn't) work(s). ;) But thanks for fixing this, anyway. *shrug*

Cheers,
-Derek