Test coverage

drunken monkey → made their first commit to this issue’s fork.

Thanks for creating this issue and helping add test coverage and proper CI to this module.

Your MR looks like a good start on the whole, thanks! However, I do have a few remarks:

• Instead of explicitly ignoring config schema problems I think it makes more sense to fix them.
• The testSettingsFormAccessible() test method seems superfluous and like it would just put unnecessary load on the CI servers. I think we should just add the single additional assertion to the other test method.
• Why did you add a version restriction to the search_api dependency? Is there actually something in that version that is needed by this module? In that case, though, we should still set 1.33 as the minimum version, as 1.32 is explicitly deprecated → .
• As mentioned in the other issue, I don’t think there’s a point to adding a composer.json file if it just replicates the information in the MODULE.info.yml file. (Also, the two should not contradict each other, like they do if we add version restrictions to just one of them.)

I added all of this to your MR. Please review!

Oh, one more thing: Do you suggest continuously testing against all those Drupal and PHP versions, or would you comment out these variables in .gitlab‎-ci.yml‎ before merging?
In my other projects, I just use these variables for testing for MRs which might impact multi-version compatibility, plus have a weekly schedule to test against all supported versions. In general, drupal.org asks to keep resource usage for CI restricted to what makes sense, so I try not to go overboard.

Thanks for reviewing this.

Instead of explicitly ignoring config schema problems I think it makes more sense to fix them.

Yes, Its just added to make sure get started with the test. Planning to fix this later.

The testSettingsFormAccessible() test method seems superfluous and like it would just put unnecessary load on the CI servers. I think we should just add the single additional assertion to the other test method.

It was added to make sure we test the searchstax settings form with multiple roles, with permission "administer searchstax", what is your thought?

Why did you add a version restriction to the search_api dependency? Is there actually something in that version that is needed by this module? In that case, though, we should still set 1.33 as the minimum version, as 1.32 is explicitly deprecated.

Sure thing. It was added as D11 support started from 1.32 version.

As mentioned in the other issue, I don’t think there’s a point to adding a composer.json file if it just replicates the information in the MODULE.info.yml file. (Also, the two should not contradict each other, like they do if we add version restrictions to just one of them.)

composer.json file added to make sure the dependencies get downloaded, currently the search_api is dependency but won't get download and module throws error see 🐛 Unable to install modules: module 'searchstax' is missing its dependency module search_api. Active .

Oh, one more thing: Do you suggest continuously testing against all those Drupal and PHP versions, or would you comment out these variables in .gitlab‎-ci.yml‎ before merging?

No, only with the Drupal core current version, it is added to make sure the test are compatible with all the version we support i.e. Previous Major, Minor and next Major, Minor.
Once the all test coverage completed and the MR gets RTBC will remove the testing against unnecessary Drupal versions.

I am also planning to add search_api_searchstax module in require-dev section and add integration testing for the same, not in this ticket maybe in separate ticket.

Updated the search_api to 1.33 version, please review 🐛 Unable to install modules: module 'searchstax' is missing its dependency module search_api. Active and merge(if looks good).

Sorry, it seems you were still working on this and didn’t want me to review yet? I apologize, I didn’t realize this until now. Lots of people forget to set the “Needs review” status, especially with the new MR system, so I’m used to reviewing MRs without that status unless the comments in the issue clearly state this is a work-in-progress.

Anyways, I have to say that it would also have made my job a lot easier if you’d used just a single issue and MR for all those issues. We now seem to be doing the same work in five different branches and I can already see us heading toward a number of avoidable merge conflicts.
Could you maybe consolidate all your work on the CI pipeline and composer.json in this issue’s MR? Then we can also merge the MR once it’s completely green, instead of some of the jobs remaining yellow for now.

It was added to make sure we test the searchstax settings form with multiple roles, with permission "administer searchstax", what is your thought?

There is no administer searchstax permission, and both of the methods used the exact same code for generating the test user. So I don’t quite understand what you mean by that.

Sure thing. It was added as D11 support started from 1.32 version.

I still don’t see the point, as users might still be on Drupal 10.2 or 10.3 at the moment. If a user wants to use this module with Drupal 11, then they’d be forced to use a version of Search API that’s compatible with that anyways, without us explicitly requiring it.

composer.json file added to make sure the dependencies get downloaded, currently the search_api is dependency but won't get download and module throws error see 🐛 Unable to install modules: module 'searchstax' is missing its dependency module search_api. Active .

Ah, yes, I already realized that the CI pipeline chokes on this for some reason. It works fine when I try it locally – as said, the Drupal Composer repository does already compute the composer.json information from the MODULE.info.yml file as far as I can see.
Still, if this is required for CI, then let’s of course add it. However, my point about not having conflicting information in composer.json and MODULE.info.yml still stands.

I'm performing a cherry-pick, which should prevent merge conflicts. I created a separate issue to ensure that if this process takes longer than expected, we can still have the other changes merged or committed promptly. This approach will facilitate the review process by allowing us to evaluate the smaller changes in one MR vs handling the larger changes.

If you believe we should continue with this single issue, I'm okay with that. We can then close the other issue as a duplicate.

Cherry-picked the commit from 📌 Test Coverage: Fix cspell warnings. Active .

Cherry-picked the commit from 📌 Test Coverage: Fix PHPCS warnings. Active .

Waiting for 📌 Test Coverage: Fix cspell warnings. Active to land.

Thomas, This one is ready for review and merge!

Great, thanks!
Merged.

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