This is a parallel issue to #2702925: Convert FlagCountsTest to a kernel test

A straight conversion of a Web test to a kernel test.

CommentFileSizeAuthor
#18 service-2703325-18.patch6.96 KBmartin107
#18 interdiff-2703325-13-18.txt5.99 KBmartin107
#13 service-2703325-13.patch7.95 KBmartin107
#13 interdiff-2703325-10-13.txt833 bytesmartin107
#10 interdiff-2703325-7-10.txt1.01 KBmartin107
#10 service-2703325-10.patch7.97 KBmartin107
#7 service-2703325-7.patch8.1 KBmartin107
#7 interdiff-2703325-5-7.txt585 bytesmartin107
#5 interdiff-2-5.txt7.63 KBmartin107
#5 service-2703325-5.patch8.05 KBmartin107
#2 service-2703325-2.patch6.55 KBmartin107

Comments

martin107 created an issue. See original summary.

martin107’s picture

martin107 commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new service-2703325-2.patch6.55 KB

Here is the conversion

I tried creating a trait, but the code was too highly coupled and dependant on all sorts of things like the $modules class variable and schema initialization
it looked a mess so I backed it out.....

For me the speed up was from 1min2s -> 28s. Nothing to sneeze at but also not that significant.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I guess the reason for that is that you change the doTest* methods to standalone test methods. Without that pattern, the test would be ~3x slower as a web test.

I agree with doing that, as the performance is still acceptable and it leads to better separation.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented 
+++ b/tests/src/Kernel/FlagServiceTest.php
@@ -2,36 +2,72 @@
+    // Create a flag.
+    $flag = Flag::create([
+      'id' => strtolower($this->randomMachineName()),
+      'label' => $this->randomString(),
+      'entity_type' => 'node',
+      'bundles' => ['article'],
+      'flag_type' => 'entity:node',
+      'link_type' => 'reload',
+      'flagTypeConfig' => [],
+      'linkTypeConfig' => [],
+    ]);

you could move this into setup, I think it's always the same?

Yeah, trait doesn' really seem worth it yet, we could make a base class with the setUp() method since that seems to be pretty much identical for both tests that we have now.

martin107’s picture

martin107 commented
StatusFileSize
new service-2703325-5.patch8.05 KB
new interdiff-2-5.txt7.63 KB

Thanks for the prompt review

We could make a base class with the setUp() method since that seems to be pretty much identical for both tests that we have now.

Yes, I took myself out for a long walk after posting the patch and was kicking myself the whole way round - because of that.

Status: Needs review » Needs work

The last submitted patch, 5: service-2703325-5.patch, failed testing.

martin107’s picture

martin107 commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2703325-5-7.txt585 bytes
new service-2703325-7.patch8.1 KB

FlagKernelTestBase is now abstract.

and I tweaked the incorrect class annonation.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

joachim’s picture

joachim commented
Status: Reviewed & tested by the community » Needs work 
+++ b/tests/src/Kernel/FlagKernelTestBase.php
@@ -0,0 +1,66 @@
+ * Basic setup for kernel tests based arround flaggings articles.

Typos here.

martin107’s picture

martin107 commented
Status: Needs work » Needs review
StatusFileSize
new service-2703325-10.patch7.97 KB
new interdiff-2703325-7-10.txt1.01 KB

arround -> around

Oh crap I misspelled that in middle school too.

I have just updated my phpcs ruleset - @file is not to be removed from class/interface/trait files.

socketwench’s picture

socketwench commented
Status: Needs review » Needs work

Everything looks good, but FlagKernelTestBase appears in the WebUI. Is there a way to prevent that? Maybe remove @group?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented 
+++ b/tests/src/Kernel/FlagKernelTestBase.php
@@ -0,0 +1,60 @@
+ *
+ * @group flag
+ */
+abstract class FlagKernelTestBase extends KernelTestBase {

Yes. Don't add the @group. Sorry, I missed that.

martin107’s picture

martin107 commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2703325-10-13.txt833 bytes
new service-2703325-13.patch7.95 KB

Good catch thanks - also added a newline expected by phpcs.

socketwench’s picture

socketwench commented

Why are we creating the flag in FlagKernelTestBase? Shouldn't this be the responsibility of the child class?

martin107’s picture

martin107 commented

Why are we creating the flag in FlagKernelTestBase? Shouldn't this be the responsibility of the child class?

What we have currently is not a pattern, I feel attached to....

So the pros and cons

1) If the pattern is 'setup supplies objects common to all test using this base' - so with a view to converting FlagCountsTest to a kernel test that extends this base class then we are in good company as FlagCountsTest::setup() provides both a Flag and a Node on tap...

2) I can see a counter example
I think
#2704959: FlagCountManager::decrementCounts() doesn't handle empty results gracefully

is a example where the test to be added might benefit from the guarantee that no stray stuff is the system before we exercise the deleting methods.... but I think this is a minor point.

@socketwench - what is you feeling on this ... do you think it puts unnecessary fluff in the setup, or is it that forces the next developer into a pattern that does not suit every situation?

martin107’s picture

martin107 commented
Related issues: +#2705449: FlagService::getFlags extend testing., +#2705443: FlagServiceTest should test FlagService::unFlagUserRemoval()

I am a paranoid bunny, who frequently jumps the gun.

So TDLR I am splitting off a couple of site issues.

Looking at the code coverage reports of my own work frequently makes me laugh out loud and cringe in equal measure.

One unstated advantage of this issue is that we can now spot holes in test coverage visually.

so to generate a report based on our 3 shiny new kernel tests type something like

cd core
../vendor/bin/phpunit --group flag --coverage-html ~/codeCoverage/

socketwench’s picture

socketwench commented
Status: Needs review » Needs work

Mostly I'm concerned that we'll run into the situation where we may need more than one flag or node in a test -- something that is the case in many of our simpletests -- and the variables will look....weird. Kernel tests may be different than our simpletests, and creating a standard flag or node out the box is just a common pattern. The thing is, we can't really know that right now. Flags and nodes are also such highly individual things that moving their creation to the child classes just makes more sense to me.

martin107’s picture

martin107 commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2703325-13-18.txt5.99 KB
new service-2703325-18.patch6.96 KB

Flags and nodes are also such highly individual things that moving their creation to the child classes just makes more sense to me.

Okeydokey

reversing #4

  • socketwench committed 1ddfcf9 on 8.x-4.x authored by martin107 
    Issue #2703325 by martin107: Changed FlagServiceTest to a kernel test.
socketwench’s picture

socketwench commented
Status: Needs review » Fixed

That works. We should also consider refactoring flag and node creation into base class methods (rather than setUp()) for child classes to call. We can do that in another issue.

Status: Fixed » Closed (fixed)

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