flag reset UI has no tests

A few things to get the tests working...

1) Node was not defined at the top of the file.
2) adminUser must be logged in before node create
3) The "are you sure message" was incorrect

Also fixed up some coding standard issues.

In answer to the question

TODO: test the API directly too?

Unit test coverage for FlagService::reset() occurs in FlagCountsTest::testFlagCounts()
Well a unit test masquerading as a WebTest!

Ah I am bit embarrassed, I didn't say thanks for doing ALL the work #4. So a belated joachim++

#6.4 I made the change only after being prompted by phpcs
a quick search for UI( in '*Test.php'

show methods like
testTranslateOperationInListUi

and in core we have lots of
public function getId() not getID()

It is in the coding standard just not uniformly enforced.

I will have more time tonight, so it is a matter of who gets there first ... can I pile another minor nit onto the list?

FlagRestTest.php there are a couple of calls to t(), when I think a call to $this->t() is better.
It my understanding that t(), as defined from bootstrap.inc is in the distant future going away.....Given that I would like not the introduce that into new test code.

Another thing I am toying with, that I don't like, is the fact that I had to use specialist knowledge of our test code base - to know where the other flag rest test code was. This is an almost fatuous point to make AND I mean this in a loving way, about most code I write - "... BUT It becomes harder and harder to unpick the spagheti"

FlagCountsTest which "Tests the Flag counts API." used to be the place for the reset test when bits of logic from both FlagCountManager and FlagService were in the same class. In a separate issue I think we should separate the tests.

From #3

Tests on my local dev seems to be taking forever,

FWIW the new test, FlagResetTest takes 1min36sec to complete on my machine.

Yeah, messes with my brains context switching ability too...

There is a testers mantra for that

Exhaustive unit testing first, limited integration testing second...

Also in a unicorn filled universe where conversion to behat is free - this is not a problem :)

Changes in the patch :-

1) From #6.1

Should probably say 'UI' as this test is only UI.

which clashes with the point from From #8

Do you mean put the test for the reset API into this test? That makes sense to me -- it's where I'd look for it.

When I wrote #7 saying separate the test ... I did mean that .... but the more I look at it the test that should be moved should go into FlagServiceTest.php
-- Well anyway that is a decision can be resolved in the follow up...

In short I have followed #6.1 as a direction.

2) #6.2 fixed.

3) #6.3 -the "| false" issue Looking at how others tests do it ..a minority of other tests do add "false".
It is technically correct -- BrowserTestBase::drupalCreateUser() will report FALSE by design and for good reasons,... which are not that relevant here. My bias is too leave it alone --- just from general experience - I like the complete approach, it is helpful when on a deadline to upset the group think - by reading annotations. :)

4) #6.4 I've reverted testFlagResetUI()

5) #7.x now using $this->t()

6) Removed the API directly TODO

Updating the issue summary with a bread crumb.

If someone use git blame on FlagReset.php - they come here...
and I want to headline that is a regression test triggered by #2548297: Replace broken call flag_reset_flag.