Port tests to 8.x-1.x

Temporarily blocked by #2851291: missing schema for storage settings.voting_tag and #3133123: Rate content permission is not ported to D8. I am working on the tests locally until then.

The situation with tests:

We have 5 test cases in D7, one of which is an Ajax test. These were written for Simpletest.

In D8, we need a port of all those functions. Plus, instead of Simpletest, we need to port the test cases to PHPUnit.

4 of the test cases should be Functional tests in D8/PHPUnit, based on BrowserTestBase, as they require browser-based interactions. The Ajax test from D7 should be a FunctionalJavascript test in D8, based on WebDriverTestBase.

What I have done is first put some of the functionality into a FivestarTestTrait so we don't have to duplicate code when writing Kernel tests (in the future) or Functional tests or FunctionalJavascript tests.

Then, I ported the tests. Two of the five work, at least locally, and are in the attached patch so I can try them out in Drupal CI. I will commit these if the pass on DrupalCI because it's important to get SOME testing working, even if it all hasn't been ported yet. That way we have a structure on which to build new tests as we finish the port of Fivestar to D8 and D9.

One of the 5 tests is the Ajax test, which I have not tried to run locally yet so there's still work to do on that.

The remaining two of the 5 tests that do NOT work are because there are some Fivestar features that haven't been ported to D8 yet - the notion of exposed ratings for one. But the missing features mean that these two tests can't work until the features are finished. I will be opening up issues for those and finishing that off, but in the meantime I'm leaving these two failing tests out.

Here is the patch containing the two working test cases.

The test in #5 failed under D8.7 because I used a new function that was introduced in D8.8, replacing an older function that won't work in D9. So the choice is to have tests that work with D8.8+ and D9, or to have tests that work with D8.7 but NOT with D9. Because D9 is going to be released in less than a month, and because these tests are still a new feature and still in the process of being ported/developed, I don't think it's worth the effort at this point to modify the tests so they will run in D8.7.

Here's a new patch which fixes the one coding standard problem found in the previous patch, and also REMOVES the old D7 test functions that are ported to D8 in this patch.

Now the error is the fault of the Voting API: see #3048085: time() is used as a callback, and I'm assuming it's php7.3 that doesn't like time having parameters.

The choices here are to make Fivestar depend on the *development* verison of Voting API (where this issue has been fixed) or to avoid using PHP 7.3+. However, since we need PHP 7.3+ for D9, I think we have to force people to use Voting API -dev.

We are reaching a point where further progress on Fivestar is being blocked by the state of the Voting API module - not just with this issue but with others as well. Hopefully we can get the Voting API maintainer to step up and fix some of these things.

Here's a first try at porting the Ajax test. I don't have chromedriver set up on my development site right now, so I'm going to use the testbot to try to debug this. Expect failures, and many iterations ...

The problem with JavaScript creating bad class names was fixed in #3137188: .star-1, .star-2 etc. classes not set correctly

I've committed the FivestarAjaxTestBase from #17 so that other people can work on Ajax tests for other features, in other issues.

This issue will remain open to complete the porting of the D7 tests, as described above.

Thanks. This clearly fell off my radar. I'll try to take a look at this sometime in the next week.

The reason I'm having trouble with this is that a lot of the changes are way out of scope - you add new features, add an update function, make unrelated coding standards changes, refactor some code, add parameters to methods, etc. Many of these are good things, but they should all be done and discussed in separate issues (and some of these separate issues already exist) rather than be mixed up here with the porting of the test functions.

I also see that you've removed working code from FivestarTest, and I can't determine whether you've replaced that with equivalent code or just deleted it. Part of this could be the refactoring you did by moving things into the test trait, but I can't tell where things went and whether we're still testing what we used to test. I spent some time working on that code and porting it from D7, and I don't want that to be thrown away. The only things that should be done here are removing the legacy D7 test functions and replacing them with new test classes/methods for D8+.

I've taken your patch from #26 and eliminated all the things I don't think belong here. Let's start from this and see if this actually works. I will iterate on this minimal patch until the tests work, then we can talk about adding things back in.

This new patch deletes the .test files and adds FivestarAjaxTest. I did not change the FivestarTestTrait, instead I put those changes directly into FivestarAjaxTest. If this works we can refactor common code into the Trait at a later date. Putting all that common setup into the Trait is very opinionated about what tests want to do. In particular, I don't understand the changes you made to FivestarTestTrait::createFivestarField(), because it appears you've taken away important options for setting the widget type etc. I would prefer putting only things like convenience functions into the trait, and leaving the setup stuff to be done as needed in any test classes or in a TestBase like they were.

I'm sure there are things I took out that are actually needed, especially a lot of the changes in FivestarTest, but as I said I am trying to get rid of the things that don't belong and add them back one by one if needed.

To summarize what is in #34:

• New test class FivestarAjaxTest was added. This subclasses FivestarAjaxTestBase and contains one test method FivestarAjaxTest::testViewerRatingAjax().
• Three new asserts were added to FivestarTestTrait. These are assertFivestarRatingOutput(), assertFivestarVotingAvailable(), and assertFivestarVotingNotAvailable().
• Two old Simpletest classes were removed.

That's all. No new module features, and no new test methods other than the one mentioned above.

What I would like to determine is, are all the D7 test methods now ported, with this patch? In 7.x-2.x I see these five test methods:

testAuthorRating()
testViewerRating()
testViewerNonRating()
testViewerRatingAjax()
testExposedWidgetDisplay()

In 8.x-1.x I see these five test methods now, with this patch:

testAuthorRating()
testViewerNonRating()
testViewerRatingAjax()
testWidgetAlter()
testWidgetDiscovery()

Retesting to see if this still passes after #3345124: 'classy' is deprecated in Drupal 9.5 and removed from Drupal 10.
Also modifying composer.json and fivestar.info.yml to see if this passes with D10.

The tests pass, using the 'classy' theme, in #34.
The failures in #36, when the theme is switched to 'stark', implies that our JavaScript relies on specific parts of the page markup that don't appear in 'stark'.

As a rule, if we require a class in the DOM in order to function correctly, we should be putting that class there ourselves and not relying on the theme to put it there.

I don't know exactly where this problem is, but this is an example of why we absolutely need to finish porting this Ajax test.

#34 is the patch that passes. The "inching" was finished last week.
Something still needs to be done about createFivestarFieldTwo() because I don't know why you changed that in a way that doesn't allow testing different widgets. This method needs to be unified with createFivestarField() and my questions from #35 need to be addressed.

#26 is basically a non-starter for the reasons I explained. It's full of out-of-scope changes, new features, and other things that need their own issues. This issue is just about porting the old tests, which was essentially finished in #17, almost three years ago, except for one failure I didn't get working.

I am all in favor of new tests, and I'm all in favor of finishing the porting of other features and adding new features and the like, but that stuff doesn't belong here because lumping a dozen unrelated things into one commit makes the code unmaintainable, and I really need this to become *more* maintainable. There are already far too many commits with no associated issue, and far too many commits that make out-of-scope changes without explanation - just figuring out what some pieces of this module are supposed to do is a major task sometimes because there is no correspondence between the changes and any discussion of those changes, because the discussion is the documentation of intent.

@heddn: As you can see from the test in https://www.drupal.org/project/fivestar/issues/3287600#comment-14945824, Fivestar can be installed in D10 and will pass tests in D10 with the changes to composer.json and fivestar.info.yml that are in that post. But that doesn't mean it will actually WORK in D10, because our test coverage is really bad. I don't want to roll a D10 release until I know that this modules basically works in D10 (it doesn't have to be perfect, but it does have to do at least something right ...)

I have added related issues to #3287600: Drupal 10 compatibility fixes for the D10 port. I think the only real issues are the jquery.once issue and the theme functions. I'm not looking for band-aid patches just to make it not-fail, but I would like to see someone put a real effort into fixing these.

Before last week, no one had actually tested this module against D10 to see if the tests would even pass. All the tests in #3287600: Drupal 10 compatibility fixes were against D9, not D10 - if they had triggered a D10 tests they would have see it fail miserably. But as of https://www.drupal.org/project/fivestar/issues/3287600#comment-14947334 we now have this module passing D10 tests. But as I said it still doesn't actually work in D10 because the tests don't test much - they're only the MINIMUM required to declare this module compatible with D10. It does you no good to have a D10 release if the module generates errors when uses try to rate content.

I've also introduced one change, adding a allow_vote setting on entity view displays. This adds back the ability to have view only widgets when viewing an entity, where rating is not allowed. I.e. the "Allow voting on the entity." checkbox. In D7 this setting was "expose". Given that's a pretty overloaded term, I've gone with "allow_vote" which seems more more self documenting.

It would be good to get this back into Fivestar. I didn't find an existing issue for this. Can you please open a new issue for just that feature, and include the tests for just that feature in that new issue?