[tests] Make add-org tests robust #880 #1019

Ajay110125yadav · 2026-01-29T06:31:00Z

Checklist

I have read the OpenWISP Contributing Guidelines
I have manually tested the changes
I have written test cases for the change

Description

Issue #880 reported that tests passed even when createConfiguration(response) was commented out, meaning the tests weren't actually verifying the core functionality.

Changes Made

Enhanced assertions in config/__tests__/add-org.test.js to verify:
- Config file is actually created on disk (fs.existsSync)
- Config file contains correct data (name, slug, etc.)
Added detailed comments explaining the verification logic

Verification

Tested by temporarily commenting out createConfiguration(response) in add-org.js:

Before fix: Tests passed (incorrect behavior)
After fix: Tests fail as expected ✅

Uncommented the line back and all tests pass.

Fixes #880

coderabbitai · 2026-01-29T06:31:21Z

📝 Walkthrough

Walkthrough

The PR modifies config/add-org.js to run interactive/script logic only when executed directly by adding a require.main === module guard and to export four public identifiers: createConfiguration, createConfigurationWithPrompts, createConfigurationWithoutPrompts, and prompts. Tests in config/tests/add-org.test.js were adjusted: a trailing space was introduced in a test fixture value and assertions were added to verify that the created organization config file exists after successful creation in two tests.

Sequence Diagram(s)

(omitted)

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly indicates the PR makes add-org tests more robust by referencing issue `#880`, accurately summarizing the main change.
Description check	✅ Passed	The description follows the template with checklist items completed, references issue `#880`, explains changes made, and includes verification steps demonstrating the fix works.
Linked Issues check	✅ Passed	The code changes address the primary objectives of issue `#880` by adding assertions to verify the config file is created and contains correct data, preventing regressions if createConfiguration is removed.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing issue `#880`; test enhancements and module export changes are both within scope of making the automated tests more robust.

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

✨ Finishing touches

📝 Generate docstrings

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d24ef35 and 0114212.

📒 Files selected for processing (2)

config/__tests__/add-org.test.js
config/add-org.js

🚧 Files skipped from review as they are similar to previous changes (1)

config/add-org.js

🧰 Additional context used

🧬 Code graph analysis (1)

config/__tests__/add-org.test.js (1)

config/add-org.js (1)

fs (1-1)

⏰ 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). (1)

GitHub Check: Tests and Coverage

🔇 Additional comments (2)

config/__tests__/add-org.test.js (2)
25-25: Clarify intentional trailing space in URL.

The trailing space in "http://127.0.0.1:8000 " appears unintentional. If this is meant to test whitespace trimming behavior, please add a comment explaining the intent. Otherwise, this could cause subtle issues if the URL is used without trimming.
🔧 Suggested fix if unintentional
-  openwisp_radius_url: "http://127.0.0.1:8000 ",
+  openwisp_radius_url: "http://127.0.0.1:8000",
71-72: Good addition to verify file creation.

Adding this assertion ensures the test validates that the configuration file is actually created before attempting duplicate slug detection. This directly addresses issue #880 by catching regressions where createConfiguration(response) might be removed.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

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

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

coderabbitai

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@config/add-org.js`:
- Around line 257-268: The module currently executes CLI logic unconditionally
by calling createConfigurationWithoutPrompts/createConfigurationWithPrompts
based on process.argv when the file is imported; wrap that argv-handling block
in a require.main === module guard so the CLI only runs when the file is
executed directly, allowing tests to import exported symbols
(createConfiguration, createConfigurationWithPrompts,
createConfigurationWithoutPrompts, prompts) without triggering prompts or
process.argv handling.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1393643 and 5f39043.

⛔ Files ignored due to path filters (4)

client/components/login/__snapshots__/login.test.js.snap is excluded by !**/*.snap
client/components/password-reset/__snapshots__/password-reset.test.js.snap is excluded by !**/*.snap
client/components/registration/__snapshots__/registration.test.js.snap is excluded by !**/*.snap
client/components/status/__snapshots__/status.test.js.snap is excluded by !**/*.snap

📒 Files selected for processing (6)

client/components/status/status.test.js
client/test-translation.json
config/__tests__/add-org.test.js
config/add-org.js
i18n/en.po
scripts/generate-test-translations.js

🧰 Additional context used

🧬 Code graph analysis (1)

config/__tests__/add-org.test.js (1)

config/add-org.js (3)

require (4-4)

require (6-6)

fs (1-1)

⏰ 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). (1)

GitHub Check: Tests and Coverage

🔇 Additional comments (6)

client/test-translation.json (1)

12-591: LGTM — updated translation map looks consistent.

The expanded key set and msgstr values match the updated translations structure.

i18n/en.po (1)

460-478: LGTM — grammar fixes read well.

client/components/status/status.test.js (1)

1501-1510: LGTM — expectation now matches the user-facing message.

config/__tests__/add-org.test.js (2)

5-58: LGTM — stronger --noprompt coverage with disk assertion and timeout.

72-86: LGTM — duplicate-slug case now validates created file and uses a timeout.

scripts/generate-test-translations.js (1)

1-83: This parser is appropriate for the en.po file format.

The en.po file contains only simple msgid/msgstr pairs and does not include msgid_plural, msgstr[n], or msgctxt entries. The parser correctly handles the actual file structure and will not drop any data.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

config/add-org.js

nemesifier

Why are you mixing commits from #1018 ?

Previously, the tests only checked the exit status of the command, which passed even when createConfiguration() was commented out. Now the tests verify that: 1. The organization config file is actually created 2. The file contains the correct data (name, slug, etc.) 3. This ensures tests will fail if createConfiguration() is accidentally removed or commented out, protecting against regression. Fixes openwisp#880

Ajay110125yadav · 2026-01-29T16:27:34Z

@nemesifier sorry both fixed now . #1019 - Cleaned up, only #880 changes (2 files).#1018 - Separate PR for #902 translations. Please review both when free.Thanks

nemesifier

I don't understand what's the point of this PR. I don't see new tests being added. The diff doesn't show significant changes, I am really puzzled.

nemesifier · 2026-01-29T16:39:51Z

config/__tests__/add-org.test.js

    expect(result.stderr).toMatch(/already exists/);
  });
-
-  it("runs interactively and creates config (smoke test)", () => {


why remove this?

@nemesifier Sorry for the confusion . i didn't new tests - i added assertions to existing tests. Specifically, in the " prevents duplicate organization slugs" test, i added expect(fs.existsSync(testOrgConfig)).toBe(true); after the first creation succeeds.this verifies createConfiguration actually created the file before testing the duplicate check. the smoke test wasn't removed - i accidentally messed up the file while cleaning git history earlier , but it's back now. the point is to catch cases where createConfiguration might be commented out or broken (issue #880).Before, tests only checked exit codes, not whether files were acually created. Learning git workflows as i go - thanks for bearing with me .

Added fs.existsSync assertion in duplicate slug test to verify createConfiguration actually creates files. Fixes openwisp#880

Ajay110125yadav · 2026-01-31T07:00:20Z

@nemesifier : review

coderabbitai bot requested changes Jan 29, 2026

View reviewed changes

config/add-org.js Outdated Show resolved Hide resolved

Ajay110125yadav force-pushed the issues/880-make-add-org-tests-robust branch 2 times, most recently from d5068f2 to 17a4803 Compare January 29, 2026 11:23

coderabbitai bot approved these changes Jan 29, 2026

View reviewed changes

Ajay110125yadav force-pushed the issues/880-make-add-org-tests-robust branch 3 times, most recently from 0da9585 to e958a9f Compare January 29, 2026 11:39

nemesifier requested changes Jan 29, 2026

View reviewed changes

Ajay110125yadav force-pushed the issues/880-make-add-org-tests-robust branch from e958a9f to d24ef35 Compare January 29, 2026 14:58

Ajay110125yadav requested a review from nemesifier January 29, 2026 16:28

nemesifier reviewed Jan 29, 2026

View reviewed changes

[tests] Add file existence check openwisp#880

0114212

Added fs.existsSync assertion in duplicate slug test to verify createConfiguration actually creates files. Fixes openwisp#880

Ajay110125yadav force-pushed the issues/880-make-add-org-tests-robust branch from d24ef35 to 0114212 Compare January 29, 2026 18:43

Ajay110125yadav requested a review from nemesifier January 29, 2026 19:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[tests] Make add-org tests robust #880 #1019

[tests] Make add-org tests robust #880 #1019

Uh oh!

Ajay110125yadav commented Jan 29, 2026

Uh oh!

coderabbitai bot commented Jan 29, 2026 •

edited

Loading

Walkthrough

Sequence Diagram(s)

Estimated code review effort

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

nemesifier left a comment

Uh oh!

Ajay110125yadav commented Jan 29, 2026

Uh oh!

nemesifier left a comment

Uh oh!

nemesifier Jan 29, 2026

Uh oh!

Ajay110125yadav Jan 29, 2026

Uh oh!

Ajay110125yadav commented Jan 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

[tests] Make add-org tests robust #880 #1019

Are you sure you want to change the base?

[tests] Make add-org tests robust #880 #1019

Uh oh!

Conversation

Ajay110125yadav commented Jan 29, 2026

Checklist

Description

Changes Made

Verification

Uh oh!

coderabbitai bot commented Jan 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Sequence Diagram(s)

Estimated code review effort

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

Ajay110125yadav commented Jan 29, 2026

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

nemesifier Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

Ajay110125yadav Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

Ajay110125yadav commented Jan 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai bot commented Jan 29, 2026 •

edited

Loading