- Issue created by @mparker17
- Status changed to Needs review
over 1 year ago 1:54pm 24 March 2023 The last submitted patch, 3: 3350211-3--test-plugin-api.patch, failed testing. View results β
- π¨π¦Canada mparker17 UTC-4
Testbot runs Drupal in
/subdirectory
, so I can't check if the attributes are exactly equal to. Switched to xpath selectors to make it easier to check if an attribute contains a value. - π¨π¦Canada ambient.impact Toronto
Ambient.Impact β made their first commit to this issueβs fork.
- Merge request !12#3350211: Add tests for the Sitemap plugin API β (Merged) created by ambient.impact
- π¨π¦Canada ambient.impact Toronto
Not sure why my review questions aren't showing up here but I added some comments in GitLab. I find that way nicer for collaboration than the patch and interdiff workflow.
- π¨π¦Canada ambient.impact Toronto
I have no idea how that happened. Derp.
- First commit to issue fork.
- last update
over 1 year ago 24 pass - π¨π¦Canada mparker17 UTC-4
Apparently I haven't been getting notifications about changes to this ticket, otherwise I would have got back to everyone much sooner! Very sorry about that!
A brief glance at the code suggests that the changes look good: thanks @ Ambient.Impact and @Diego_Mow!
That being said, since working on the patch in #5, I've learned that string translation is not recommended in tests... the only proof I have of this right now is this Drupal Answers answer, but I seem to recall learning that this was a case via failing lints from either PHPCS-with- Coder β , or PHPStan-with-phpstan-drupal, or Rector-with-drupal-rector or similar. I can't seem to find that right now, though.
I'm working on getting Drupal.org's GitLab CI β running on this project, so I'll probably try to do that before merging this, so we can see if the lints pick up anything from this merge request.
- π¨π¦Canada ambient.impact Toronto
@mparker17
That being said, since working on the patch in #5, I've learned that string translation is not recommended in tests...
That's been my general sense that you shouldn't use translation for the actual test classes unless you're testing the translation system, but it may make sense to do in test modules because that's closer to the stuff being tested rather than the stuff performing the test, if that logic makes sense.
I'm working on getting Drupal.org's GitLab CI running on this project, so I'll probably try to do that before merging this, so we can see if the lints pick up anything from this merge request.
I've got it set up on a couple of contrib projects now, so feel free to ping me if you need help.
- last update
10 months ago 24 pass - π¨π¦Canada mparker17 UTC-4
mparker17 β changed the visibility of the branch 8.x-2.x to hidden.
- Status changed to Fixed
10 months ago 1:18pm 2 March 2024 - π¨π¦Canada mparker17 UTC-4
This looks good to me, so I have merged. Thank you very much, @Ambient.Impact and @Diego_Mow!
-
mparker17 β
committed 7392b841 on 8.x-2.x authored by
Ambient.Impact β
#3350211: Add tests for the Sitemap plugin API
-
mparker17 β
committed 7392b841 on 8.x-2.x authored by
Ambient.Impact β
Automatically closed - issue fixed for 2 weeks with no activity.
- π¨π¦Canada mparker17 UTC-4
Note that this change was released in sitemap-8.x-2.0-beta5 β .