Conversation
…tionality and Editor modal interactions
…emaining shortcodes
WalkthroughAdds CI/CD workflows, developer tooling, WordPress test environment, PHPUnit bootstrap and many unit and E2E tests, repository and review configuration, and documentation updates to enable automated testing and deployment for the Bootstrap Shortcodes plugin. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Sinetheta
force-pushed
the
feature/ci-cd-setup
branch
from
3096214 to
99acc03
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
.github/workflows/deploy.yml (1)
3-6: Consider requiring tests to pass before deployment.The deploy workflow triggers on any tag push independently of test results. You might want to add a dependency on the test workflow or run tests within this workflow before deploying.
💡 Optional: Add test job as dependency
on: push: tags: - "*" + workflow_run: + workflows: ["Test"] + types: + - completed jobs: deploy: runs-on: ubuntu-latest + # Only deploy if tests passed (when triggered by workflow_run) + if: ${{ github.event_name == 'push' || github.event.workflow_run.conclusion == 'success' }}Alternatively, you could duplicate the test steps in this workflow before the deploy step.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy.yml around lines 3 - 6, The deploy workflow is triggered on any tag push ("on: push: tags: - \"*\"") without gating on tests; update the workflow to require tests pass by either making the deploy job depend on an existing test job (add needs: test to the deploy job) or add a test job that runs your test steps prior to deployment and have the deploy job depend on it; reference the deploy job name (e.g., deploy) and the test job name you create (e.g., test) when wiring the dependency so deployment runs only after successful test completion..github/workflows/test.yml (2)
11-13: Consider adding a job timeout.Without a timeout, hung Docker containers or flaky tests could consume CI minutes indefinitely.
⏱️ Proposed fix
test: runs-on: ubuntu-latest + timeout-minutes: 30🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test.yml around lines 11 - 13, Add a timeout to the CI job to prevent hung builds from running indefinitely: inside the jobs block for the test job (job id "test") add the GitHub Actions timeout setting (timeout-minutes) with an appropriate value (e.g., 10-30) so the workflow will cancel the job after the specified minutes; update the job definition that currently includes "runs-on: ubuntu-latest" to also include the timeout-minutes property.
44-45: Consider uploading Playwright artifacts on failure.E2E test failures are difficult to debug without screenshots, traces, or HTML reports. Adding an artifact upload step will significantly improve debugging capabilities.
📦 Proposed addition after E2E tests
- name: Upload Playwright Report uses: actions/upload-artifact@v4 if: failure() with: name: playwright-report path: playwright-report/ retention-days: 7🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test.yml around lines 44 - 45, Add an artifact upload step immediately after the "Run E2E Tests" step in the workflow (.github/workflows/test.yml) to upload Playwright artifacts on failure; specifically add a step using actions/upload-artifact@v4 with if: failure(), name set to "playwright-report", path "playwright-report/" and retention-days 7 so screenshots/traces/HTML reports are preserved for debugging when the npm run test:e2e step fails.package.json (1)
5-5: Consider removing or updating themainfield.This is a WordPress PHP plugin, not a Node.js module. The
main: "index.js"field is misleading since no such entry point exists. Consider removing this field or setting it to an empty string.🧹 Proposed fix
- "main": "index.js",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 5, The package.json currently specifies a Node entry point via the "main" field pointing to "index.js", which is incorrect for this WordPress PHP plugin; remove the "main" property or set it to an empty string so it no longer advertises a non-existent Node entry (reference the "main" key and the "index.js" value in package.json). Update package.json accordingly and run a quick npm/yarn lint or package validation to ensure no tooling relies on that field.specs/editor.spec.js (1)
49-66: Consider extracting repeated setup code to a helper.The Visual mode switch and kitchen sink toggle logic is duplicated across all tests. Extracting this to a helper function or extending
beforeEachwould improve maintainability.♻️ Proposed refactor - extract helper
async function ensureVisualModeWithKitchenSink(page) { const htmlMode = await page.locator('#wp-content-wrap.html-active').count(); if (htmlMode > 0) { await page.locator('#content-tmce').click(); } await page.waitForSelector('#wp-content-wrap.tmce-active'); const isKitchenSinkActive = await page.locator('.mce-active[aria-label^="Toolbar Toggle"]').count(); if (isKitchenSinkActive === 0) { const toggleButton = page.locator('div[aria-label^="Toolbar Toggle"] button').first(); if (await toggleButton.isVisible()) { await toggleButton.click(); } } }Then use in tests:
test('Should insert a Button shortcode using the modal', async ({ admin, page, editor }) => { await ensureVisualModeWithKitchenSink(page); // ... rest of test });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/editor.spec.js` around lines 49 - 66, The test suite repeats the Visual mode switch and kitchen-sink toggle logic across tests; extract that logic into a reusable helper (e.g., ensureVisualModeWithKitchenSink) and call it from each test or from a beforeEach hook to reduce duplication; locate the repeated code using the selectors '#wp-content-wrap.html-active', '#content-tmce', '#wp-content-wrap.tmce-active' and the kitchen-sink checks using '.mce-active[aria-label^="Toolbar Toggle"]' and 'div[aria-label^="Toolbar Toggle"] button' and move that sequence into the new helper, then replace the in-test blocks in test('Should insert a Button shortcode using the modal', ...) and other tests with a single await ensureVisualModeWithKitchenSink(page) call (or call it from beforeEach) so behavior remains identical but centralized.tests/phpunit/ShortcodeCollapseTest.php (1)
19-20: Assertions depend on exact whitespace, which makes tests brittle.Lines 19 and 20 assert specific whitespace patterns (trailing space in class attribute, multiple spaces before
</div>). While these match the current implementation, they're fragile—any refactoring to clean up whitespace in the shortcode output will break these tests.Consider using regex-based assertions or splitting into multiple assertions that ignore insignificant whitespace:
♻️ Suggested refactor using regex
- $this->assertStringContainsString('<div id="c1" class="panel-collapse collapse " role="tabpanel" aria-labelledby="heading_c1">', $output); - $this->assertStringContainsString('<div class="panel-body">Inner Content </div>', $output); + $this->assertMatchesRegularExpression('/<div id="c1" class="panel-collapse collapse\s*"/', $output); + $this->assertStringContainsString('class="panel-body"', $output); + $this->assertStringContainsString('Inner Content', $output);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpunit/ShortcodeCollapseTest.php` around lines 19 - 20, The assertions in ShortcodeCollapseTest.php rely on exact whitespace in the rendered $output and are brittle; update the two assertions that currently use assertStringContainsString with exact-space strings to either use regex-based assertions (assertMatchesRegularExpression) that allow flexible whitespace (e.g. \s+ or \s* where spaces appear) or normalize the $output first (collapse all consecutive whitespace to a single space via preg_replace('/\s+/', ' ', $output')) and then assert substrings without significant whitespace; change the checks that reference '<div id="c1" class="panel-collapse collapse " role="tabpanel" aria-labelledby="heading_c1">' and '<div class="panel-body">Inner Content </div>' accordingly so they no longer depend on exact spacing.tests/phpunit/ShortcodeTabsTest.php (1)
45-45: Test expects unquoted HTML attribute value.The assertion expects
id=home>without quotes around the attribute value. While technically valid HTML5 for simple values, this is inconsistent with other assertions in the test suite that expect quoted attributes. This may indicate inconsistent output from the shortcode implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpunit/ShortcodeTabsTest.php` at line 45, The test assertion in ShortcodeTabsTest currently expects an unquoted attribute value ('<div class="tab-pane fade active in" id=home>'); update the expected string to use quoted attribute values (e.g. '<div class="tab-pane fade active in" id="home">') so it matches the rest of the suite and the shortcode output, and scan nearby assertions in the same test (and related tests) to make them consistent with quoted attributes as needed.tests/phpunit/ShortcodeAlertsTest.php (1)
18-25: Consider using@dataProviderfor better test isolation.The loop approach works, but if one type fails, the test stops without checking remaining types. Using a data provider gives clearer failure messages and tests each type independently.
♻️ Proposed refactor using `@dataProvider`
- public function test_alert_types() { - $types = array( 'success', 'info', 'warning', 'danger' ); - - foreach ( $types as $type ) { - $output = do_shortcode( '[bs_notification type="' . $type . '"]Test[/bs_notification]' ); - $this->assertStringContainsString( 'alert-' . $type, $output ); - } + /** + * `@dataProvider` alert_types_provider + */ + public function test_alert_types( $type ) { + $output = do_shortcode( '[bs_notification type="' . $type . '"]Test[/bs_notification]' ); + $this->assertStringContainsString( 'alert-' . $type, $output ); + } + + public function alert_types_provider() { + return array( + 'success' => array( 'success' ), + 'info' => array( 'info' ), + 'warning' => array( 'warning' ), + 'danger' => array( 'danger' ), + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpunit/ShortcodeAlertsTest.php` around lines 18 - 25, Replace the single loop test in test_alert_types with a data-provider based test to isolate failures: add a public static/provider method (e.g., alertTypeProvider) that yields each alert type ('success','info','warning','danger'), annotate test_alert_types with `@dataProvider` alertTypeProvider, and change test_alert_types to accept a single $type parameter and assert that do_shortcode('[bs_notification type="'.$type.'"]Test[/bs_notification]') contains 'alert-'.$type; this ensures each type runs as an independent test case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 22-25: The devDependencies entry pins `@wordpress/env` at ^10.39.0
and should be updated to ^11.1.0; open package.json, update the "@wordpress/env"
version string in the "devDependencies" section to ^11.1.0, run npm/yarn
install, then run your project's test/build/dev workflow to verify compatibility
with the existing "@wordpress/scripts" ^31.6.0 and fix any breaking changes
reported during startup or tests (adjust config or downgrade if necessary).
In `@specs/editor.spec.js`:
- Around line 15-28: Remove the temporary debug file writes by deleting the
require('fs') import and both fs.writeFileSync('dom-debug.html', html) and
fs.writeFileSync('dom-debug-after.html', html2) calls in the test; also remove
the now-unused local variables html and html2 (from the page.content() calls) or
reuse them if needed for assertions, and keep the mode-switching logic using
page.locator('#wp-content-wrap.html-active') and page.locator('#content-tmce')
intact.
In `@specs/plugin.spec.js`:
- Around line 19-20: The regex /active/ matches "inactive"; update the assertion
on pluginRow to ensure only the standalone class "active" is matched by using a
word-boundary regex like /\bactive\b/ with toHaveClass (or add an explicit
negative assertion using pluginRow.not.toHaveClass(/inactive/) after the
positive check). Target the existing toHaveClass call on pluginRow to make this
change.
In `@tests/phpunit/ShortcodeButtonsTest.php`:
- Line 13: The bs_buttons() shortcode is emitting duplicate classes because both
$size and $type default to "default" and both "btn-{$size}" and "btn-{$type}"
are appended; update bs_buttons() to prevent emitting identical size/type
classes—either make $size default to '' (no class) or add logic that only
appends the size class when $size is non-empty and different from $type (e.g.,
if ($size && $size !== $type) add "btn-{$size}"), so the resulting class list
never contains duplicate "btn-default".
In `@tests/phpunit/ShortcodeGridTest.php`:
- Around line 37-42: The regex in test_nested_grid (inside
ShortcodeGridTest::test_nested_grid) uses .*? which doesn't match newlines and
can fail if the generated HTML contains line breaks; update the assertion in
test_nested_grid (the call to $this->assertMatchesRegularExpression) to use the
DOTALL modifier (or replace .*? with [\s\S]*?) so the pattern matches across
newlines, ensuring the row/col structure check succeeds even when output
contains newlines.
In `@tests/phpunit/ShortcodeTabsTest.php`:
- Line 30: The test is asserting invalid HTML because the bs_dropdown shortcode
implementation in inc/bs_tabs.php (bs_dropdown) outputs two class attributes by
concatenating separate class strings; update bs_dropdown to merge the classes
into a single class attribute (combine esc_attr($class) with "dropdown-toggle")
so the anchor has one class attribute, then update the expectation in
tests/phpunit/ShortcodeTabsTest.php (the assertion around $output) to assert the
corrected markup (single class attribute with both classes, id="opts" and the
rest unchanged).
In `@tests/phpunit/ShortcodeWellsTest.php`:
- Line 8: Change the default size in inc/bs_well.php from the non-standard
'unknown' to an empty string (i.e., ensure the $size default is ''), and make
the markup generation omit any "-{size}" suffix so the base class becomes just
"well"; then update the test in ShortcodeWellsTest.php to assert '<div
class="well">' instead of '<div class="well-unknown">'. Reference the $size
variable and the function that renders the well in inc/bs_well.php when making
the change.
---
Nitpick comments:
In @.github/workflows/deploy.yml:
- Around line 3-6: The deploy workflow is triggered on any tag push ("on: push:
tags: - \"*\"") without gating on tests; update the workflow to require tests
pass by either making the deploy job depend on an existing test job (add needs:
test to the deploy job) or add a test job that runs your test steps prior to
deployment and have the deploy job depend on it; reference the deploy job name
(e.g., deploy) and the test job name you create (e.g., test) when wiring the
dependency so deployment runs only after successful test completion.
In @.github/workflows/test.yml:
- Around line 11-13: Add a timeout to the CI job to prevent hung builds from
running indefinitely: inside the jobs block for the test job (job id "test") add
the GitHub Actions timeout setting (timeout-minutes) with an appropriate value
(e.g., 10-30) so the workflow will cancel the job after the specified minutes;
update the job definition that currently includes "runs-on: ubuntu-latest" to
also include the timeout-minutes property.
- Around line 44-45: Add an artifact upload step immediately after the "Run E2E
Tests" step in the workflow (.github/workflows/test.yml) to upload Playwright
artifacts on failure; specifically add a step using actions/upload-artifact@v4
with if: failure(), name set to "playwright-report", path "playwright-report/"
and retention-days 7 so screenshots/traces/HTML reports are preserved for
debugging when the npm run test:e2e step fails.
In `@package.json`:
- Line 5: The package.json currently specifies a Node entry point via the "main"
field pointing to "index.js", which is incorrect for this WordPress PHP plugin;
remove the "main" property or set it to an empty string so it no longer
advertises a non-existent Node entry (reference the "main" key and the
"index.js" value in package.json). Update package.json accordingly and run a
quick npm/yarn lint or package validation to ensure no tooling relies on that
field.
In `@specs/editor.spec.js`:
- Around line 49-66: The test suite repeats the Visual mode switch and
kitchen-sink toggle logic across tests; extract that logic into a reusable
helper (e.g., ensureVisualModeWithKitchenSink) and call it from each test or
from a beforeEach hook to reduce duplication; locate the repeated code using the
selectors '#wp-content-wrap.html-active', '#content-tmce',
'#wp-content-wrap.tmce-active' and the kitchen-sink checks using
'.mce-active[aria-label^="Toolbar Toggle"]' and 'div[aria-label^="Toolbar
Toggle"] button' and move that sequence into the new helper, then replace the
in-test blocks in test('Should insert a Button shortcode using the modal', ...)
and other tests with a single await ensureVisualModeWithKitchenSink(page) call
(or call it from beforeEach) so behavior remains identical but centralized.
In `@tests/phpunit/ShortcodeAlertsTest.php`:
- Around line 18-25: Replace the single loop test in test_alert_types with a
data-provider based test to isolate failures: add a public static/provider
method (e.g., alertTypeProvider) that yields each alert type
('success','info','warning','danger'), annotate test_alert_types with
`@dataProvider` alertTypeProvider, and change test_alert_types to accept a single
$type parameter and assert that do_shortcode('[bs_notification
type="'.$type.'"]Test[/bs_notification]') contains 'alert-'.$type; this ensures
each type runs as an independent test case.
In `@tests/phpunit/ShortcodeCollapseTest.php`:
- Around line 19-20: The assertions in ShortcodeCollapseTest.php rely on exact
whitespace in the rendered $output and are brittle; update the two assertions
that currently use assertStringContainsString with exact-space strings to either
use regex-based assertions (assertMatchesRegularExpression) that allow flexible
whitespace (e.g. \s+ or \s* where spaces appear) or normalize the $output first
(collapse all consecutive whitespace to a single space via preg_replace('/\s+/',
' ', $output')) and then assert substrings without significant whitespace;
change the checks that reference '<div id="c1" class="panel-collapse collapse "
role="tabpanel" aria-labelledby="heading_c1">' and '<div
class="panel-body">Inner Content </div>' accordingly so they no longer
depend on exact spacing.
In `@tests/phpunit/ShortcodeTabsTest.php`:
- Line 45: The test assertion in ShortcodeTabsTest currently expects an unquoted
attribute value ('<div class="tab-pane fade active in" id=home>'); update the
expected string to use quoted attribute values (e.g. '<div class="tab-pane fade
active in" id="home">') so it matches the rest of the suite and the shortcode
output, and scan nearby assertions in the same test (and related tests) to make
them consistent with quoted attributes as needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b96342a9-7448-44a5-a2ba-13d5743813ee
⛔ Files ignored due to path filters (2)
composer.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
.coderabbit.yaml.cursorrules.gitattributes.github/workflows/deploy.yml.github/workflows/test.yml.gitignore.wp-env.jsonREADME.mdcomposer.jsonfuture_work.mdpackage.jsonphpunit.xml.distplaywright.config.jsspecs/editor.spec.jsspecs/plugin.spec.jstests/phpunit/PluginActiveTest.phptests/phpunit/ShortcodeAlertsTest.phptests/phpunit/ShortcodeButtonsTest.phptests/phpunit/ShortcodeCollapseTest.phptests/phpunit/ShortcodeGridTest.phptests/phpunit/ShortcodeIconsTest.phptests/phpunit/ShortcodeLabelsTest.phptests/phpunit/ShortcodeLeadTest.phptests/phpunit/ShortcodeTabsTest.phptests/phpunit/ShortcodeTooltipTest.phptests/phpunit/ShortcodeWellsTest.phptests/phpunit/bootstrap.php
💤 Files with no reviewable changes (1)
- future_work.md
Comment on lines
+22
to
+25
| "devDependencies": { | ||
| "@wordpress/env": "^10.39.0", | ||
| "@wordpress/scripts": "^31.6.0" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check latest versions of WordPress packages
echo "Checking `@wordpress/env`..."
npm view `@wordpress/env` version
echo "Checking `@wordpress/scripts`..."
npm view `@wordpress/scripts` versionRepository: TheWebShop/bootstrap-shortcodes
Length of output: 146
Update @wordpress/env to version 11.x.
@wordpress/env has a major version update available (currently pinned to ^10.39.0, latest is 11.1.0). @wordpress/scripts (31.6.0) is current. Evaluate upgrading @wordpress/env to version 11.1.0 and test for compatibility with your WordPress setup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 22 - 25, The devDependencies entry pins
`@wordpress/env` at ^10.39.0 and should be updated to ^11.1.0; open package.json,
update the "@wordpress/env" version string in the "devDependencies" section to
^11.1.0, run npm/yarn install, then run your project's test/build/dev workflow
to verify compatibility with the existing "@wordpress/scripts" ^31.6.0 and fix
any breaking changes reported during startup or tests (adjust config or
downgrade if necessary).
Comment on lines
+15
to
+28
| // Dump HTML for debugging | ||
| const html = await page.content(); | ||
| const fs = require('fs'); | ||
| fs.writeFileSync('dom-debug.html', html); | ||
|
|
||
| // Switch to Visual mode if not already there, since WP remembers mode per user | ||
| const htmlMode = await page.locator('#wp-content-wrap.html-active').count(); | ||
| if (htmlMode > 0) { | ||
| await page.locator('#content-tmce').click(); | ||
| } | ||
|
|
||
| // Dump HTML for debugging AFTER click | ||
| const html2 = await page.content(); | ||
| fs.writeFileSync('dom-debug-after.html', html2); |
There was a problem hiding this comment.
Remove debug file writes before merging.
The fs.writeFileSync calls writing dom-debug.html and dom-debug-after.html are debug artifacts that should not run in CI. They create unnecessary files in the workspace and may cause issues in restricted environments.
🧹 Proposed fix - remove debug statements
test('Should render Bootstrap Shortcodes buttons in the toolbar', async ({ admin, page, editor }) => {
- // Dump HTML for debugging
- const html = await page.content();
- const fs = require('fs');
- fs.writeFileSync('dom-debug.html', html);
-
// Switch to Visual mode if not already there, since WP remembers mode per user
const htmlMode = await page.locator('#wp-content-wrap.html-active').count();
if (htmlMode > 0) {
await page.locator('#content-tmce').click();
}
- // Dump HTML for debugging AFTER click
- const html2 = await page.content();
- fs.writeFileSync('dom-debug-after.html', html2);
-
// Wait for the TinyMCE editor to be ready📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Dump HTML for debugging | |
| const html = await page.content(); | |
| const fs = require('fs'); | |
| fs.writeFileSync('dom-debug.html', html); | |
| // Switch to Visual mode if not already there, since WP remembers mode per user | |
| const htmlMode = await page.locator('#wp-content-wrap.html-active').count(); | |
| if (htmlMode > 0) { | |
| await page.locator('#content-tmce').click(); | |
| } | |
| // Dump HTML for debugging AFTER click | |
| const html2 = await page.content(); | |
| fs.writeFileSync('dom-debug-after.html', html2); | |
| // Switch to Visual mode if not already there, since WP remembers mode per user | |
| const htmlMode = await page.locator('#wp-content-wrap.html-active').count(); | |
| if (htmlMode > 0) { | |
| await page.locator('#content-tmce').click(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/editor.spec.js` around lines 15 - 28, Remove the temporary debug file
writes by deleting the require('fs') import and both
fs.writeFileSync('dom-debug.html', html) and
fs.writeFileSync('dom-debug-after.html', html2) calls in the test; also remove
the now-unused local variables html and html2 (from the page.content() calls) or
reuse them if needed for assertions, and keep the mode-switching logic using
page.locator('#wp-content-wrap.html-active') and page.locator('#content-tmce')
intact.
| public function test_default_button() { | ||
| $output = do_shortcode( '[bs_button]' ); | ||
|
|
||
| $this->assertStringContainsString( '<a class="btn btn-default btn-default" href="#">button</a>', $output ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for bs_button shortcode implementation
rg -n 'bs_button' --type php -A 15 -B 2Repository: TheWebShop/bootstrap-shortcodes
Length of output: 3146
Shortcode implementation produces duplicate btn-default class.
The bs_buttons() function concatenates both a size class (btn-{$size}) and a type class (btn-{$type}). When both $size and $type default to 'default', the output contains class="btn btn-default btn-default". Fix the shortcode to avoid applying both size and type classes, or use distinct defaults that don't collide.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/phpunit/ShortcodeButtonsTest.php` at line 13, The bs_buttons()
shortcode is emitting duplicate classes because both $size and $type default to
"default" and both "btn-{$size}" and "btn-{$type}" are appended; update
bs_buttons() to prevent emitting identical size/type classes—either make $size
default to '' (no class) or add logic that only appends the size class when
$size is non-empty and different from $type (e.g., if ($size && $size !== $type)
add "btn-{$size}"), so the resulting class list never contains duplicate
"btn-default".
| $content = '[bs_dropdown title="Options" id="opts" class="my-drop"][bs_tab href="#opt1" title="Opt 1"][/bs_dropdown]'; | ||
| $output = do_shortcode($content); | ||
| $this->assertStringContainsString('<li class="dropdown">', $output); | ||
| $this->assertStringContainsString('<a class="my-drop" id="opts" class="dropdown-toggle" data-toggle="dropdown">Options<b class="caret"></b></a>', $output); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the bs_dropdown shortcode implementation to verify the duplicate class issue
ast-grep --pattern 'add_shortcode($_, $_)'
rg -n 'bs_dropdown' --type php -A 10Repository: TheWebShop/bootstrap-shortcodes
Length of output: 4238
Test documents invalid HTML output from buggy bs_dropdown shortcode implementation.
The assertion at line 30 expects class="my-drop" id="opts" class="dropdown-toggle" — two class attributes on the same element. This is invalid HTML and confirms a bug in the shortcode implementation at inc/bs_tabs.php:59, where the code concatenates two separate class attributes instead of combining them:
$result .= '<a class="' . esc_attr($class) . '" id="' . esc_attr($id) . '" class="dropdown-toggle" data-toggle="dropdown">' . esc_attr($title) . '<b class="caret"></b></a>';The classes should be merged into a single attribute. The test is locking in this buggy behavior rather than testing correct output.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/phpunit/ShortcodeTabsTest.php` at line 30, The test is asserting
invalid HTML because the bs_dropdown shortcode implementation in inc/bs_tabs.php
(bs_dropdown) outputs two class attributes by concatenating separate class
strings; update bs_dropdown to merge the classes into a single class attribute
(combine esc_attr($class) with "dropdown-toggle") so the anchor has one class
attribute, then update the expectation in tests/phpunit/ShortcodeTabsTest.php
(the assertion around $output) to assert the corrected markup (single class
attribute with both classes, id="opts" and the rest unchanged).
| public function test_well_default() { | ||
| $content = '[bs_well]Inner Content[/bs_well]'; | ||
| $output = do_shortcode($content); | ||
| $this->assertStringContainsString('<div class="well well-unknown">', $output); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for bs_well shortcode implementation to check default size handling
rg -n 'bs_well' --type php -A 15Repository: TheWebShop/bootstrap-shortcodes
Length of output: 2632
Shortcode implementation has buggy default size value.
The test correctly reflects the actual behavior: inc/bs_well.php line 4 sets the default size to 'unknown', which produces the non-standard class well-unknown. This is not a Bootstrap size class and appears unintentional. The implementation should default to a valid size like 'md' or an empty string. Update inc/bs_well.php to fix the default, then update this test assertion accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/phpunit/ShortcodeWellsTest.php` at line 8, Change the default size in
inc/bs_well.php from the non-standard 'unknown' to an empty string (i.e., ensure
the $size default is ''), and make the markup generation omit any "-{size}"
suffix so the base class becomes just "well"; then update the test in
ShortcodeWellsTest.php to assert '<div class="well">' instead of '<div
class="well-unknown">'. Reference the $size variable and the function that
renders the well in inc/bs_well.php when making the change.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
specs/plugin.spec.js (1)
11-17: Consider adding an explicit existence check for the plugin row.If the plugin slug changes or the plugin isn't installed, the test will fail with a less informative timeout error. Adding an explicit visibility assertion before interacting provides clearer failure messages.
💡 Suggested improvement
const pluginRow = page.locator('tr[data-slug="bootstrap-shortcodes"]'); + await expect(pluginRow).toBeVisible(); // If the plugin happens to be inactive, ensure we can activate it const activateLink = pluginRow.locator('.activate a');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/plugin.spec.js` around lines 11 - 17, The test assumes pluginRow ('tr[data-slug="bootstrap-shortcodes"]') exists which can produce opaque timeouts; add an explicit existence/visibility assertion for pluginRow before interacting. Locate the pluginRow locator and call an assertion (e.g., expect(pluginRow).toBeVisible() or await pluginRow.waitFor({ state: 'visible' })) to fail fast with a clear message if the plugin is missing, then proceed to check activateLink and click as currently implemented.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@specs/plugin.spec.js`:
- Around line 11-17: The test assumes pluginRow
('tr[data-slug="bootstrap-shortcodes"]') exists which can produce opaque
timeouts; add an explicit existence/visibility assertion for pluginRow before
interacting. Locate the pluginRow locator and call an assertion (e.g.,
expect(pluginRow).toBeVisible() or await pluginRow.waitFor({ state: 'visible'
})) to fail fast with a clear message if the plugin is missing, then proceed to
check activateLink and click as currently implemented.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6b0c1112-439f-4522-9211-6d9fc863a710
📒 Files selected for processing (2)
specs/plugin.spec.jstests/phpunit/ShortcodeGridTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/phpunit/ShortcodeGridTest.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Description
Description
This PR introduces automated testing and Continuous Integration (CI) to the
bootstrap-shortcodesplugin. It sets up both a PHPUnit test suite for backend logic and a Playwright End-to-End (E2E) test suite to verify plugin activation and editor integration.Additionally, GitHub Actions workflows have been created to run tests on all pull requests and pushes to
master, as well as a deployment workflow to automatically release tagged versions to the WordPress.org SVN repository.Related Issues
Changes Made
package.jsonfor@wordpress/scriptsand@wordpress/env, andcomposer.jsonfor PHPUnit polyfills.phpunit.xml.distand initialization intests/phpunit/bootstrap.php. Added a basic activation test instance.specs/plugin.spec.jsto ensure the plugin can install, activate, and render without fatal errors..github/workflows/test.ymlfor automated Github Actions testing and.github/workflows/deploy.ymlfor SVN releases.How to Test
npm installandcomposer install.npx @wordpress/env start.npm run test:php.npm run test:e2e.Summary by CodeRabbit
New Features
Tests
Documentation
Chores