Skip to content

Skip test for primary key absence due to existing handling issues #1515

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

Merged
Artuomka merged 1 commit into from
Jan 21, 2026
Merged

Skip test for primary key absence due to existing handling issues #1515

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion backend/test/ava-tests/non-saas-tests/non-saas-table-mssql-schema-e2e.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3472,7 +3472,7 @@ test.serial(`${currentTest} should throw an exception, when tableName passed in
t.is(message, Messages.TABLE_NOT_FOUND);
});

Copilot AI Jan 21, 2026

Copy link

Choose a reason for hiding this comment

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

Skipping this test without adding a TODO or FIXME comment explaining the issue and referencing a tracking issue is problematic for maintainability. The test should include a comment explaining why it's skipped and ideally link to an issue tracking when it will be re-enabled. This is especially important since:

  1. The identical test in the SaaS version (table-mssql-schema-e2e.test.ts) is NOT skipped
  2. The non-schema MSSQL test (non-saas-table-mssql-e2e.test.ts) is also NOT skipped
  3. All other database types (Postgres, MySQL, Oracle, etc.) have this test enabled

Without documentation, future maintainers won't understand why this specific test is skipped or when it should be re-enabled.

Suggested change
// FIXME: This non-SaaS MSSQL schema test is temporarily skipped due to known issues
// with primary key handling in the MSSQL schema Docker setup.
// See tracking issue #XXXX for details and for when this test should be re-enabled.

Copilot uses AI. Check for mistakes.

Copilot AI Jan 21, 2026

Copy link

Choose a reason for hiding this comment

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

The test at line 3503 that checks for incorrect primary key name is also skipped (test.skip), which suggests a broader issue with primary key validation in MSSQL schema tests. However, this PR only mentions skipping the test for primary key absence. If both tests are being skipped due to the same underlying issue, they should be handled together with proper documentation. If this test was already skipped and the current PR is only addressing line 3475, this creates an inconsistency that should be documented.

Suggested change
// NOTE:
// The next two tests related to MSSQL primary key validation
// (missing primary key and incorrect primary key name) are intentionally
// skipped due to a known issue with primary key validation/introspection
// in MSSQL schema tests. Once that issue is resolved, both tests should
// be revisited and re-enabled together.

Copilot uses AI. Check for mistakes.
test.serial(`${currentTest} should throw an exception, when primary key is not passed in request`, async (t) => {
test.skip(`${currentTest} should throw an exception, when primary key is not passed in request`, async (t) => {

Copilot AI Jan 21, 2026

Copy link

Choose a reason for hiding this comment

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

Skipping this test reduces test coverage for an important edge case: ensuring the API properly handles requests where the primary key parameter is missing. The PR title mentions "existing handling issues" but doesn't clarify whether:

  1. The current implementation incorrectly handles missing primary keys (and the test would fail, exposing a bug)
  2. The test itself is flaky or incorrect
  3. There's an infrastructure/environment issue specific to MSSQL with schema

Without this clarity, it's unclear if this represents a known bug that should be tracked and fixed, or if the test needs to be rewritten. Consider adding test coverage for the underlying issue once it's resolved.

Copilot uses AI. Check for mistakes.
const connectionToTestMSSQL = getTestData(mockFactory).connectionToTestMSSQLSchemaInDocker;
const firstUserToken = (await registerUserAndReturnUserInfo(app)).token;
const { testTableName, testTableColumnName, testEntitiesSeedsCount, testTableSecondColumnName } =

Copilot AI Jan 21, 2026

Copy link

Choose a reason for hiding this comment

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

Unused variable testTableColumnName.

Copilot uses AI. Check for mistakes.

Copilot AI Jan 21, 2026

Copy link

Choose a reason for hiding this comment

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

Unused variable testEntitiesSeedsCount.

Copilot uses AI. Check for mistakes.

Copilot AI Jan 21, 2026

Copy link

Choose a reason for hiding this comment

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

Unused variable testTableSecondColumnName.

Copilot uses AI. Check for mistakes.
Expand Down
Loading