Port tests to 8.x-1.x

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.

The last submitted patch, 27: 3131954-27-port-d7-tests.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 29: 3131954-29-port-d7-tests.patch, failed testing. View results →

The last submitted patch, 32: 3131954-33-port-d7-tests.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

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 📌 'classy' is deprecated in Drupal 9.5 and removed from Drupal 10 Fixed .
Also modifying composer.json and fivestar.info.yml to see if this passes with D10.

The last submitted patch, 36: 3131954-34-port-d7-tests-with-d10.patch, failed testing. View results →

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.

Giving this a new title given the the calendar date has flown away and this won't make it into D8 as much as the 8.x-1.x branch.

This, and votingapi are my last 2 modules that don't have D10 releases. I see some very massive work has gone into things the past few days. Yeah!!!

I can't get a feel for how close this is to landing so we can fully support D10. Does this just need some minor fixes that should happen in the next few days or is this weeks away? Either extreme is fine and I want to emphasize my appreciation for all the great work to make this module D10 compatible.

And mention 📌 Remove 'fivestar_preview' theme function Needs work as something else that is needed.

Hi @TR, thanks for your work on this.

I have to say it's a little frustrating given that you're inching your way back to the passing tests I had in #26. When you've reached some finishing point I'll be happy to review it and compare it to what I had, and also run my project specific tests which are built on top of #26.

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?

@fenstrat I wonder if the fixes in #3324052-7: Deprecated function addcslashes() warning caused by empty value → are a better way to handle the context information in the render element. Can you weigh in over there please?

This takes the tests only from #36 and makes one small CSS change due to changes in stark default theme. Its passes locally on 10.2, hopefully also on 9.5?

Yeah, Green!

Landing this so we can improve upon it in other issues.

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

@heddn and @TR thanks for all your work here!

I've created a follow up to further improve test coverage: 📌 Improve test coverage and add back "Allow voting" display option Needs review as a lot of the tests from #26 didn't end up making it in here. 3447756 also adds back the allow_vote option which didn't make it in here either.

Sorry for the radio silence in all the follow ups post #26 - the project I needed this on stalled for a long time. Thankfully it's ramped back up so I should be able to dedicate a bit of time to helping polish up fivestar.