[fix] Handle skipped Selenium tests in Django parallel runner

[czarflix]

Checklist

• I have read the OpenWISP Contributing Guidelines.
• I have manually tested the changes proposed in this pull request.
• I have written new test cases for new code and/or updated existing tests for changes to existing code.
• I have updated the documentation.

Reference to Existing Issue

Closes #619.

Description of Changes

openwisp-utils#570 fixed skipped Selenium tests by propagating skips from result.skipped.

That works for standard unittest results, but it breaks under Django's parallel test runner because RemoteTestResult.skipped is a truthy DummyList, which is not iterable. In that case, skip information is recorded in result.events.

This patch keeps the serial skip behavior introduced by #570 and fixes the parallel case by reading skip reasons from result.events when available, while falling back to result.skipped for standard results.

It also adds focused regression coverage for both result types:

• standard TestResult
• Django RemoteTestResult

Verification

I verified locally with:

• ./run-qa-checks
• python tests/manage.py test test_project.tests.test_selenium_mixin
• focused parallel runs of test_project.tests.test_selenium_mixin under Django 4.2, 5.0, 5.1, and 5.2

I also checked the behavior around #570 directly:

• before #570, skipped Selenium tests were incorrectly reported as successful
• #570 fixed serial skip propagation but crashes under the parallel result type
• this patch preserves the serial fix and handles the parallel result correctly

Screenshot

N/A

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6c719dcd-5cbb-48f1-81bc-9b7efea8dec8

📥 Commits

Reviewing files that changed from the base of the PR and between e89df63 and c7a2856.

📒 Files selected for processing (2)

• openwisp_utils/tests/selenium.py
• tests/test_project/tests/test_selenium_skip.py

📜 Recent review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)

• GitHub Check: changelog / generate-changelog
• GitHub Check: Python==3.12 | django~=5.0.0
• GitHub Check: Python==3.13 | django~=5.1.0
• GitHub Check: Python==3.12 | django~=5.1.0
• GitHub Check: Python==3.13 | django~=5.2.0
• GitHub Check: Python==3.12 | django~=4.2.0
• GitHub Check: Python==3.10 | django~=5.1.0
• GitHub Check: Python==3.12 | django~=5.2.0
• GitHub Check: Python==3.11 | django~=5.2.0
• GitHub Check: Python==3.10 | django~=4.2.0
• GitHub Check: Python==3.11 | django~=5.0.0
• GitHub Check: Python==3.11 | django~=4.2.0
• GitHub Check: Python==3.10 | django~=5.2.0
• GitHub Check: Python==3.11 | django~=5.1.0
• GitHub Check: Python==3.10 | django~=5.0.0

🧰 Additional context used

📓 Path-based instructions (1)

**/*.{py,html,txt}

📄 CodeRabbit inference engine (Custom checks)

For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework

Files:

• openwisp_utils/tests/selenium.py
• tests/test_project/tests/test_selenium_skip.py

🧠 Learnings (4)

📓 Common learnings

Learnt from: CR
Repo: openwisp/openwisp-utils PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T20:44:14.568Z
Learning: Bug Fixes: If the bug affects the user interface, include a Selenium browser test; if missing, raise a warning

Learnt from: CR
Repo: openwisp/openwisp-utils PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T20:44:14.568Z
Learning: Bug Fixes: Ensure the test is deterministic and not flaky - flag tests that depend on timing, sleeps, specific timezones, system time, randomness without fixed seed, race conditions, concurrency timing, network access, external services, filesystem state, environment-specific configuration, execution order, shared global state, hardcoded ports, or unawaited async operations

📚 Learning: 2026-03-14T20:44:14.568Z

Learnt from: CR
Repo: openwisp/openwisp-utils PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T20:44:14.568Z
Learning: Features: Add tests for new features and ensure coverage does not decrease significantly; prefer Selenium browser tests for UI-impacting features

Applied to files:

• openwisp_utils/tests/selenium.py
• tests/test_project/tests/test_selenium_skip.py

📚 Learning: 2026-03-14T20:44:14.568Z

Learnt from: CR
Repo: openwisp/openwisp-utils PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T20:44:14.568Z
Learning: Bug Fixes: If the bug affects the user interface, include a Selenium browser test; if missing, raise a warning

Applied to files:

• openwisp_utils/tests/selenium.py

📚 Learning: 2026-03-14T20:44:14.568Z

Learnt from: CR
Repo: openwisp/openwisp-utils PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T20:44:14.568Z
Learning: Changes: Update tests to cover non-trivial changes and ensure proper validation of modified behavior

Applied to files:

• tests/test_project/tests/test_selenium_skip.py

🔇 Additional comments (2)

openwisp_utils/tests/selenium.py (1)

57-67: LGTM! Skip propagation logic is correct and handles both result types properly.

The implementation correctly:

1. Detects RemoteTestResult via hasattr(result, "events") and extracts skip reasons from addSkip events
2. Falls back to result.skipped for standard TestResult objects
3. Guards with if skip_reasons: to avoid premature return for non-skipped remote results
4. Calls original_result.stopTest(self) to properly record the stop event at the correct test index, avoiding the shared event log corruption issue

tests/test_project/tests/test_selenium_skip.py (1)

1-69: LGTM! Comprehensive regression tests covering both standard and parallel runner scenarios.

The test design is solid:

• _run_skipped_standard_test correctly bypasses setUpClass by calling _setup_and_call directly, testing the result.skipped fallback path
• _run_skipped_remote_suite properly provides DummyWebDriver since RemoteTestRunner.run() invokes the full test lifecycle
• Testing two skipped methods validates correct index assignment (0, 1) and proves the shared RemoteTestResult isn't corrupted
• The exact events assertion [("startTest", 0), ("addSkip", 0, ...), ("stopTest", 0), ...] confirms proper event sequencing per test

The tests are deterministic with no timing or external dependencies. Based on learnings: "Bug Fixes: Ensure the test is deterministic and not flaky."

---

📝 Walkthrough

Walkthrough

The changes fix a crash that occurs when skipped Selenium tests run under Django's parallel test runner. The fix modifies skip propagation logic in SeleniumTestMixin._setup_and_call() to derive skip reasons from result.events when available (as in Django's RemoteTestResult), otherwise falling back to result.skipped for standard unittest results. When skip reasons are found, each is propagated to the original result and the test is stopped. Comprehensive tests are added to verify correct skip propagation in both standard and parallel runner scenarios.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

• [change] Handle skipped tests in SeleniumTestMixin #570: Introduced the initial skip-propagation logic to SeleniumTestMixin by forwarding skip reasons to the original result and halting retry logic; this PR extends that implementation to handle Django's RemoteTestResult object format.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title follows the required [type] format with [fix] prefix and clearly summarizes the main change: handling skipped Selenium tests in Django's parallel runner.
Description check	✅ Passed	The description comprehensively covers all critical sections: completed checklist items, issue reference (#619), detailed explanation of changes with context, and verification steps performed.
Linked Issues check	✅ Passed	The code changes fully address issue #619 objectives: skip handling now reads from result.events for Django's parallel runner while preserving #570's serial behavior, with regression tests covering both standard and RemoteTestResult scenarios.
Out of Scope Changes check	✅ Passed	All changes are scoped to fixing skip handling in SeleniumTestMixin and adding focused regression tests; no unrelated modifications detected.
Bug Fixes	✅ Passed	Pull request properly fixes the root cause of SeleniumTestMixin._setup_and_call() crash when Selenium test was skipped under Django's parallel runner by detecting RemoteTestResult and reading skip reasons from result.events instead of result.skipped.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[czarflix]

@asmodehn Should I push the changes ?

[czarflix]

@asmodehn @nemesifier

I pushed the cleanup changes for the agreed review points:

• simplified the skip-source selection logic
• removed the unnecessary setUpClass() / tearDownClass() overrides in the regression test

I kept the if skip_reasons: guard because it protects the skip-only early return for RemoteTestResult, not just the loop.

[nemesifier]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[czarflix]

hi @nemesifier

@coderabbitai review

All Green

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get the current state of the changed files
cat openwisp_utils/tests/selenium.py
echo "---FILE_SEPARATOR---"
cat tests/test_project/tests/test_selenium_mixin.py

Length of output: 13978

---

🏁 Script executed:

#!/bin/bash
# Check Django's RemoteTestResult implementation to confirm event format
python -c "import inspect; from django.test.runner import RemoteTestResult; print(inspect.getsource(RemoteTestResult))" 2>/dev/null || \
  pip show django -q && python -c "import django; print(django.__version__)"

# Also search pip-installed django source
find / -path "*/django/test/runner.py" 2>/dev/null | head -3 | xargs grep -A5 "def addSkip\|def startTest\|test_index" 2>/dev/null | head -60

Length of output: 37

---

🌐 Web query:

Django RemoteTestResult addSkip startTest test_index events format runner.py

💡 Result:

In Django’s parallel test runner, the subprocess uses django.test.runner.RemoteTestResult to record a linear “event log” (result.events) that the parent process can replay. Each entry is a tuple whose first item is an event name, followed by event-specific payload. (docs.djangoproject.com)

test_index

RemoteTestResult.test_index is defined as:

• test_index = self.testsRun - 1 (i.e., 0-based index of the “current” test as unittest increments testsRun in startTest). (docs.djangoproject.com)

Event tuple formats (as appended by RemoteTestResult)

• ("startTestRun",) (docs.djangoproject.com)
• ("stopTestRun",) (docs.djangoproject.com)
• ("startTest", test_index) (docs.djangoproject.com)
• ("stopTest", test_index) (docs.djangoproject.com)
• ("addDuration", test_index, elapsed_seconds) (docs.djangoproject.com)
• ("addSuccess", test_index) (docs.djangoproject.com)
• ("addSkip", test_index, reason_string) (docs.djangoproject.com)
• ("addError", test_index, err) where err is the sys.exc_info()-style tuple (docs.djangoproject.com)
• ("addFailure", test_index, err) (docs.djangoproject.com)
• ("addSubTest", test_index, subtest_obj, err) (only recorded when err is not None) (docs.djangoproject.com)
• ("addExpectedFailure", test_index, err) (docs.djangoproject.com)
• ("addUnexpectedSuccess", test_index) (docs.djangoproject.com)

Where it’s returned from the worker

The worker returns (subsuite_index, result.events) back to the parent process. (docs.djangoproject.com)

Citations:

• 1: https://docs.djangoproject.com/en/5.0/_modules/django/test/runner/
• 2: https://docs.djangoproject.com/en/5.0/_modules/django/test/runner/
• 3: https://docs.djangoproject.com/en/5.0/_modules/django/test/runner/
• 4: https://docs.djangoproject.com/en/5.0/_modules/django/test/runner/
• 5: https://docs.djangoproject.com/en/5.0/_modules/django/test/runner/
• 6: https://docs.djangoproject.com/en/5.0/_modules/django/test/runner/
• 7: https://docs.djangoproject.com/en/5.0/_modules/django/test/runner/
• 8: https://docs.djangoproject.com/en/5.0/_modules/django/test/runner/
• 9: https://docs.djangoproject.com/en/5.0/_modules/django/test/runner/
• 10: https://docs.djangoproject.com/en/5.0/_modules/django/test/runner/
• 11: https://docs.djangoproject.com/en/5.0/_modules/django/test/runner/
• 12: https://docs.djangoproject.com/en/5.0/_modules/django/test/runner/
• 13: https://docs.djangoproject.com/en/5.0/_modules/django/test/runner/
• 14: https://docs.djangoproject.com/en/5.0/_modules/django/test/runner/
• 15: https://docs.djangoproject.com/en/5.0/_modules/django/test/runner/

---

@czarflix Thanks for the cleanup work — the simplified skip-source selection logic is clean. The dual-path detection (result.events vs result.skipped) and the if skip_reasons: guard look correct. There is one concern worth discussing before merging:

---

original_result.events = result.events — event log corruption for multi-method test cases

In Django's parallel runner, RemoteTestResult.test_index is testsRun - 1 (a 0-based index of the current test, incremented by each startTest call). A single RemoteTestResult instance accumulates events for all test methods that run in a given worker. The worker ultimately returns (subsuite_index, result.events) back to the parent process, so the integrity of the full event log matters.

In _setup_and_call, a fresh inner result object is created per attempt (always starting with test_index = 0). By the time this line executes:

original_result.events = result.events

original_result.events already contains correctly-indexed events from all previously run test methods in that worker, plus the ("startTest", N) for the current test (added at the top of _setup_and_call), plus the ("addSkip", N, reason) that was just added by original_result.addSkip(self, reason) on the line above.

The overwrite replaces all of that with the inner result's events, which only contain 3 entries with index 0:

[("startTest", 0), ("addSkip", 0, reason), ("stopTest", 0)]

The intent appears to be capturing the ("stopTest", ...) event (since the return prevents stopTest from being called on original_result otherwise). A safer approach that achieves that without clobbering prior events would be:

if hasattr(original_result, "events"):
    original_result.stopTest(self)  # records ("stopTest", test_index) at the correct index

And remove the original_result.events = result.events block entirely. Since addSkip already recorded the skip event with the correct index on the line above, there's nothing else to copy.

Why the regression test doesn't catch this: _run_skipped_test creates a brand-new result_class instance and runs a single test, so test_index is always 0 in both the outer and inner result. The bug only manifests when a skip occurs in a test method that isn't the first one run by that worker.

[czarflix]

Follow-up pushed. The event-log corruption concern was correct: the skip path was overwriting the shared RemoteTestResult event log.

I removed that overwrite, record stopTest() on the outer remote result instead, and updated the regression coverage to run multiple skipped methods from the same SeleniumTestMixin class through RemoteTestRunner, so the shared-worker case is now covered.

[coveralls]

coverage: 97.476% (+0.1%) from 97.348%
when pulling c7a2856 on czarflix:fix/selenium-parallel-skip-handling
into e1d24be on openwisp:master.

[czarflix]

Hi @nemesifier @asmodehn . All changes addressed, please review once you have the time

[czarflix]

@nemesifier could you take a look, this addresses the selenium test issue and it unlocks failing CIs of the other issues

[nemesifier]

Code looks good but manual testing is needed before confirming and merging.

[asmodehn]

Looks like a good and clean set of changes with matching test. Thanks !

[czarflix]

Looks like a good and clean set of changes with matching test. Thanks !

@nemesifier , please take a look when you have the time

[czarflix]

@nemesifier
For manual verification, I tested the skip path locally in both serial and Django parallel runs, including the shared RemoteTestResult case with multiple skipped methods in the same class. The behavior is now correct in both cases and the shared event log is preserved. If you want, I can rerun any specific scenario.

[nemesifier]

I did some minor changes, the most important one being to always call original_result.stopTest(self) when the test has to be skipped, while before this call was conditional, which didn't make sense: if it's skipped it should be always be stopped, right? Let me know if that's not the case @czarflix @asmodehn.

[czarflix]

I did some minor changes, the most important one being to always call original_result.stopTest(self) when the test has to be skipped, while before this call was conditional, which didn't make sense: if it's skipped it should be always be stopped, right? Let me know if that's not the case @czarflix @asmodehn.

I think this change is correct.

The key point is that _setup_and_call() manually calls original_result.startTest(self) before the retry loop, and neither SimpleTestCase.__call__() nor TestSuite.run() will balance that on the outer result for us later. In the normal unittest contract, a skipped test still goes through startTest -> addSkip -> stopTest, and for RemoteTestResult the stopTest() call is also what records the final ("stopTest", index) event.

So I don’t see a case where a propagated skip on original_result should skip stopTest().