Skip to content

test: redo cockroachdb to service-container GHA and update its version to latest (24) #11190

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
michaelbromley merged 26 commits into from
Dec 16, 2024
Merged

test: redo cockroachdb to service-container GHA and update its version to latest (24) #11190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
9a8e78e
comment out cockroachdb setup for new database
OSA413 Dec 13, 2024
d36acde
decrease size of cache and memory to 1GB for cockroachdb
OSA413 Dec 13, 2024
3c3c8f2
test: cockroachdb memory 4GB
OSA413 Dec 13, 2024
7ea031b
test: Redo cockroachdb to using services
OSA413 Dec 13, 2024
6f7513d
test: cockroach remove momery args
OSA413 Dec 13, 2024
5e4cc03
test: cockroach just start, not single node
OSA413 Dec 13, 2024
a59681a
rewind
OSA413 Dec 13, 2024
36858ef
Let's see if this works
OSA413 Dec 13, 2024
1e2d415
Checking one theory
OSA413 Dec 13, 2024
7ee4905
cockroachdb:v23.1.9
OSA413 Dec 13, 2024
d0a7d08
try this
OSA413 Dec 13, 2024
4493267
let's see what's wrong with the new version
OSA413 Dec 13, 2024
7718e73
let's see if this helps
OSA413 Dec 13, 2024
2e0f676
test: update CRDB test after update to 24.2
OSA413 Dec 14, 2024
a5951da
will this fix CRDB transaction fail in test
OSA413 Dec 14, 2024
29c303b
revert
OSA413 Dec 14, 2024
bdd36c3
try fix rollback
OSA413 Dec 14, 2024
c5ef5db
fix parenthesis
OSA413 Dec 14, 2024
1f3737d
???
OSA413 Dec 14, 2024
c649300
see if this cockroach_restart helps
OSA413 Dec 14, 2024
8d81914
final touches
OSA413 Dec 14, 2024
a2619de
Update cockroachdb.ormconfig.json
OSA413 Dec 14, 2024
b36fbd3
apply formatting
OSA413 Dec 14, 2024
54452a7
Add fix for CLI tests
OSA413 Dec 14, 2024
20eb464
try this
OSA413 Dec 15, 2024
38b96ca
Added comment about disabled code
OSA413 Dec 15, 2024
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
14 changes: 9 additions & 5 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,20 @@ jobs:
strategy:
fail-fast: false
matrix:
node-version: [16.x, 18.x, 20.x] #, 22.x]
node-container: ["node:16", "node:18", "node:20"] #, "node:22"]
# See supported Node.js release schedule at https://nodejs.org/en/about/releases/

container: ${{ matrix.node-container }}
services:
crdb:
image: cockroachdb/cockroach
env:
COCKROACH_ARGS: 'start-single-node --insecure --cache=1GB --store=type=mem,size=4GB'

steps:
- uses: actions/setup-node@v4
with:
node-version: ${{matrix.node-version}}
- uses: actions/checkout@v4
- run: docker compose -f .github/workflows/test/cockroachdb.docker-compose up -d
- run: npm i
- run: chown -R 1001:127 /github/home/.npm #This fix is needed for running CLI tests
- run: cp .github/workflows/test/cockroachdb.ormconfig.json ormconfig.json
- run: npm test

Expand Down
8 changes: 0 additions & 8 deletions .github/workflows/test/cockroachdb.docker-compose
View file
Open in desktop

This file was deleted.

5 changes: 3 additions & 2 deletions .github/workflows/test/cockroachdb.ormconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,12 @@
"skip": false,
"name": "cockroachdb",
"type": "cockroachdb",
"host": "localhost",
"host": "crdb",
"port": 26257,
"username": "root",
"password": "",
"database": "defaultdb"
"database": "defaultdb",
"logging": false
},

{
Expand Down
8 changes: 5 additions & 3 deletions src/driver/cockroachdb/CockroachQueryRunner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,9 @@ export class CockroachQueryRunner
} else {
this.storeQueries = false
this.transactionDepth -= 1
await this.query("RELEASE SAVEPOINT cockroach_restart")
// This was disabled because it failed tests after update to CRDB 24.2
// https://github.com/typeorm/typeorm/pull/11190
// await this.query("RELEASE SAVEPOINT cockroach_restart")
await this.query("COMMIT")
this.queries = []
this.isTransactionActive = false
Expand Down Expand Up @@ -1019,7 +1021,7 @@ export class CockroachQueryRunner
const enumColumns = newTable.columns.filter(
(column) => column.type === "enum" || column.type === "simple-enum",
)
for (let column of enumColumns) {
for (const column of enumColumns) {
// skip renaming for user-defined enum name
if (column.enumName) continue

Expand Down Expand Up @@ -3903,7 +3905,7 @@ export class CockroachQueryRunner
table: Table,
indexOrName: TableIndex | TableUnique | string,
): Query {
let indexName =
const indexName =
InstanceChecker.isTableIndex(indexOrName) ||
InstanceChecker.isTableUnique(indexOrName)
? indexOrName.name
Expand Down
5 changes: 4 additions & 1 deletion test/functional/query-builder/locking/query-builder-locking.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -975,7 +975,10 @@ describe("query builder > locking", () => {
.createQueryBuilder(Post, "post")
.leftJoin("post.author", "user")
.setLock("pessimistic_write")
.getOne(),
.getOne()
.should.be.rejectedWith(
Copy link
Collaborator

@alumni alumni Jan 27, 2025

Choose a reason for hiding this comment

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

This seems to be breaking the transaction and making RELEASE SAVEPOINT unavailable: https://www.cockroachlabs.com/docs/stable/release-savepoint#handling-errors

Because Chai does not forward the error, Promise.all will not throw either, making EntityManager.transaction think the operation was successful, so it won't roll back.

We should probably make that function fail.

Copy link
Collaborator

@alumni alumni Jan 27, 2025

Choose a reason for hiding this comment

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

Yepp, it's the same thing in the privious test ;) There's this comment:

https://github.com/typeorm/typeorm/pull/11190/files#diff-0c1916cf8da19e54f4ed20c05df0044acf71abe15bc7db30e4275ba35ec13b17L953

"FOR UPDATE cannot be applied to the nullable side of an outer join",
),
])
})
}
Expand Down
16 changes: 11 additions & 5 deletions test/functional/repository/find-options-locking/find-options-locking.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -638,11 +638,17 @@ describe("repository > find options > locking", () => {
tables: ["post"],
},
}),
entityManager.getRepository(Post).findOne({
where: { id: 1 },
relations: { author: true },
lock: { mode: "pessimistic_write" },
}),
entityManager
.getRepository(Post)
.findOne({
where: { id: 1 },
relations: { author: true },
lock: { mode: "pessimistic_write" },
})
.should.be.rejectedWith(
"FOR UPDATE cannot be applied to the nullable side of an outer join",
),
,
])
})
}
Expand Down
Loading