Skip to content

Tests #113

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
anishapant21 wants to merge 39 commits into
base: dev
Choose a base branch
from
Open

Tests #113

anishapant21 wants to merge 39 commits into from

Conversation

@anishapant21
Copy link
Collaborator

@anishapant21 anishapant21 commented Dec 11, 2025 •
edited
Loading

Test Infrastructure

  • Testing Pyramid Structure: Organized tests into unit, integration, and e2e
  • Centralized Test Data: Created test/data/ directory with JSON/SQL files for all backends
  • Data Loading Utilities: Built dataLoader.js and dbSeeder.js for consistent test data
  • CI/CD Integration: Added GitHub Actions workflows for automated testing

Test Coverage

  • Unit Tests: Core utilities (ldapUtils, filterUtils, errorUtils), LdapEngine, provider interfaces
  • Integration Tests: SQL, MongoDB, Proxmox auth/directory providers with real backends
  • E2E Tests: Full SSSD client authentication with Docker Compose setup

Test Organization

  • Modular Structure: Separate npm package tests vs server tests
  • Fixtures & Mocks: Reusable mock providers and test data
  • Backend-Specific: Dedicated test suites for each backend (SQL, MongoDB, Proxmox)
# Run all tests
npm test

# Run by workspace
npm test -w npm        # Core package tests
npm test -w server     # Server tests

# Run specific backends
cd server
npm run test:integration -- auth/sql.auth.test.js
npm run test:integration -- directory/mongodb.directory.test.js

# Coverage report
npm run test:coverage -w npm
npm run test:coverage -w server

@anishapant21 anishapant21 requested a review from Copilot December 16, 2025 18:41
Copilot AI reviewed Dec 16, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds comprehensive test infrastructure for the LDAP gateway project, implementing unit tests, integration tests, and SSSD integration tests. The changes include Jest configuration, test utilities, mock providers, and extensive test coverage for authentication, directory backends, and security features.

Key Changes

  • Added Jest test framework configuration for both npm core package and server
  • Created test utilities (TestServer, LdapTestClient, dbSeeder, mockLogger)
  • Implemented unit tests for utilities, interfaces, and LdapEngine
  • Added integration tests for SQL, MongoDB, and Proxmox backends
  • Added SSSD integration tests with Docker Compose setup

Reviewed changes

Copilot reviewed 43 out of 44 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
npm/jest.config.js Jest configuration for core package with coverage thresholds
npm/package.json Added Jest dependencies and test scripts
npm/test/unit/utils/*.test.js Unit tests for ldapUtils, filterUtils, errorUtils
npm/test/unit/interfaces/*.test.js Unit tests for AuthProvider and DirectoryProvider
npm/test/unit/LdapEngine.test.js Unit tests for LDAP engine lifecycle
npm/test/fixtures/*.js Mock providers and shared test data fixtures
server/jest.config.js Jest configuration for server integration tests
server/package.json Added Jest and moved sequelize/sqlite to devDependencies
server/test/setup.js Global test setup for server integration tests
server/test/utils/*.js Test utilities (TestServer, LdapTestClient, dbSeeder, mockLogger)
server/test/fixtures/testData.js Shared test data for integration tests
server/test/integration/auth/*.test.js Auth backend integration tests (SQL, MongoDB, Proxmox)
server/test/integration/directory/*.test.js Directory backend integration tests
server/test/integration/engine/*.test.js Engine-level integration tests
server/test/integration/security/*.test.js TLS policy and security tests
server/test/integration-sssd/* SSSD integration test setup with Docker
server/utils/ldapUtils.js Updated defaults for mail and surname fields
server/backends/proxmox.*.js Added enabled/expire field support
server/db/drivers/mongoDb.js Added connection checks and field normalization
npm/src/utils/ldapUtils.js Support for both mail/email fields and login_shell
npm/src/utils/filterUtils.js Fixed cn= filter handling for groups
npm/src/LdapEngine.js Added cn filtering in mixed search requests
.github/workflows/ci.yml Added MongoDB service and SSSD integration tests

@anishapant21 anishapant21 marked this pull request as ready for review December 18, 2025 22:51
@runleveldev
Copy link
Contributor

runleveldev commented Dec 19, 2025

This is big so I'm reviewing it at a very high level here:

Starting here, only the boxed test sounds worthwhile IMO, the rest test implementation details which may change.

image

I'm not sure any of these tests are worth it, they all seem like implementation details

image

Same here, these are all implementation details most of which are enforced by Node and not worth testing anyways

image

I actually like most of the following tests, but the boxed 2 I want to change the API contract on. The directory provider MUST provide uid and gid numbers to this function or we should consider it an error.

image

Again I don't think these are worth testing, they are mostly implementation details that may be subject to change.

image

These seem fine but I would like to see the LDAP filter being used in the test output personally.

image

These look good:

image

The boxed test here seems oddly specific. What's it really testing for?

image

Why are these "mocked"? We should at the very least be doing a REAL test against SQLite but preferably also dockerized mysql and postgresql databases.

image

These look fine, I will need to do a deeper dive into the test harness to ensure they're testing things correctly. I also thought we weren't using the expired field at all

image

I was running a mongodb in Docker similar to how the PR shows. The directory tests are passing

image

But all auth tests failed with the same error

    thrown: "Exceeded timeout of 30000 ms for a hook.
    Add a timeout value to this test to increase the timeout, if this is a long-running test. See https://jestjs.io/docs/api#testname-fn-timeout."

      44 |   };
      45 |
    > 46 |   beforeAll(async () => {
         |   ^
      47 |     // Check if MongoDB is available
      48 |     const testClient = new MongoClient(mongoConfig.uri, { serverSelectionTimeoutMS: 2000 });
      49 |     try {

      at test/integration/auth/mongodb.auth.test.js:46:3
      at Object.<anonymous> (test/integration/auth/mongodb.auth.test.js:30:1)

runleveldev
runleveldev requested changes Dec 29, 2025
View reviewed changes
Copy link
Contributor

@runleveldev runleveldev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part 1 review, npm package.

README.md
npm run test:db:mysql # MySQL integration tests
npm run test:db:postgres # PostgreSQL integration tests
npm run test:db:mongodb # MongoDB integration tests
```
Copy link
Contributor

@runleveldev runleveldev Dec 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All 6 commands listed above are only valid within the server/ directory. This section of the README should either (a) be moved to server/README.md OR (b) reference the fact that the commands are only valid within the server/ directory (such as via a cd command).

npm/test/unit/utils/ldapUtils.test.js
Comment on lines +117 to +133
test('should work with all test fixture users', () => {
testUsers.forEach(user => {
const entry = createLdapEntry({
username: user.username,
uid_number: user.uid,
gid_number: user.gidNumber,
full_name: user.cn,
last_name: user.sn,
mail: user.mail,
home_directory: user.homeDirectory
}, baseDN);

expect(entry.dn).toContain(user.username);
expect(entry.attributes.uid).toBe(user.username);
expect(entry.attributes.uidNumber).toBe(user.uid);
});
});
Copy link
Contributor

@runleveldev runleveldev Dec 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exactly does this test that the previous tests miss? If nothing, remove it. If this tests something specific, be clear about that in the test description.

npm/test/unit/utils/ldapUtils.test.js
expect(entry.attributes.objectClass).toEqual(['posixGroup']);
expect(entry.attributes.cn).toBe('developers');
expect(entry.attributes.gidNumber).toBe(1002);
expect(entry.attributes.memberUid).toEqual(['alice', 'bob']);
Copy link
Contributor

@runleveldev runleveldev Dec 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just putting this comment here to voice my concern that we don't take a strong stance on memberUid vs. member. We really should pick one and support it based on whichever LDAP schema we're trying to support instead of having to test for all possible combinations. If this is the only comment that goes unaddressed I won't block this PR, but I will file a seperate issue to be more clear about which group style we support.

npm/test/unit/utils/ldapUtils.test.js
Comment on lines +257 to +270
test('should work with all test fixture groups', () => {
testGroups.forEach(group => {
const entry = createLdapGroupEntry({
name: group.cn,
gid_number: group.gidNumber,
memberUids: group.memberUids
}, baseDN);

expect(entry.dn).toContain(group.cn);
expect(entry.attributes.cn).toBe(group.cn);
expect(entry.attributes.gidNumber).toBe(group.gidNumber);
});
});
});
Copy link
Contributor

@runleveldev runleveldev Dec 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, what is this testing that the other tests don't? Remove it or be clear.

npm/test/unit/utils/ldapUtils.test.js
expect(domain).toBe('company.org');
});

test('should return localhost for invalid base DN', () => {
Copy link
Contributor

@runleveldev runleveldev Dec 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should throw an error on invalid DN rather than returning a probably wrong result.

npm/test/unit/utils/ldapUtils.test.js
expect(domain).toBe('localhost');
});

test('should return localhost for empty base DN', () => {
Copy link
Contributor

@runleveldev runleveldev Dec 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty base would mean empty domain to me. Localhost seems like the wrong default. Or we should throw an error that an empty DN was provided.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@runleveldev runleveldev runleveldev requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@anishapant21 @runleveldev