You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
refactor(test): replace findOne with findOneOrFail to eliminate non-null assertions - #12325
Replace findOneBy(), findOne(), and getOne() with their OrFail variants across the test suite, addressing the most common SonarQube feedback — unnecessary non-null assertions. Related to #12255.
The OrFail methods throw EntityNotFoundError instead of returning null, which:
Removes the need for ! type assertions on every property access
Produces clearer error messages when a test fails (entity not found vs null property access)
Removes redundant expect(x).to.not.be.null checks (the OrFail method already guarantees non-null)
Also simplifies findOneOrFail({ where: {...} }) to findOneByOrFail({...}) and findOne({ where: {...} }) to findOneBy({...}) where no other options (relations, select, order) are used.
Tests that intentionally verify null results (e.g., after transaction rollbacks or inner joins with no matches) are left unchanged with findOne() / getOne().
Also fixes loadedfeature → loadedFeature camelCase naming in geometry column tests and replaces remaining non-null assertions on nullable properties with optional chaining.
query-builder-joins.test.ts switched multiple negative-path join queries from getOne() to
getOneOrFail() while still asserting loadedPost* is null, so these tests will throw
EntityNotFoundError instead of returning null.
Each highlighted test constructs a query that should return no rows and asserts the result is
null, but getOneOrFail() throws when no rows exist. This contradiction occurs in several places
in the same file.
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
## Issue description
Several negative-path join tests expect `getOne()` to return `null` when joins/conditions eliminate results, but the refactor changed them to `getOneOrFail()`, which throws.
### Issue Context
`getOneOrFail()` throws `EntityNotFoundError` when no entity is found; these tests are explicitly validating the "no result" behavior.
### Fix Focus Areas
- test/functional/query-builder/join/query-builder-joins.test.ts[349-364]
- test/functional/query-builder/join/query-builder-joins.test.ts[1068-1089]
- test/functional/query-builder/join/query-builder-joins.test.ts[1345-1373]
### Suggested change
For each test that asserts `expect(loadedPost...).to.be.null`, change the corresponding `.getOneOrFail()` back to `.getOne()` (or update the tests to expect a thrown error if the intent is to validate `getOneOrFail`).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
2. Rollback check uses OrFail ☑ 🐞≡
Description
test/functional/transaction/transaction-in-entity-manager/transaction-in-entity-manager.test.ts
verifies a failed transaction was rolled back by expecting post/category to be null, but it
now calls findOneOrFail() for both. This will throw EntityNotFoundError instead of returning
null, breaking the rollback verification.
After the transaction error is caught, the test expects both entities to be missing (to.be.null)
but uses findOneOrFail(), which rejects when the entity is not found (per the
EntityManager.findOneOrFail implementation).
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
## Issue description
Rollback verification expects `null`, but `findOneOrFail()` throws when the record is missing.
### Issue Context
This test intentionally checks that after a failed transaction, persisted entities are rolled back and cannot be found.
### Fix Focus Areas
- test/functional/transaction/transaction-in-entity-manager/transaction-in-entity-manager.test.ts[114-125]
### Suggested fix
- Replace the rollback verification queries with `findOne()`/`findOneBy()` and keep `expect(x).to.be.null`.
- Or, keep `findOneOrFail()` and assert that the promise is rejected with `EntityNotFoundError` (and remove the `to.be.null` assertions).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
3. Non-null assertions remain ☑ 🐞⚙
Description
findOneOrFail is used, but loadedPerson1! / loadedPerson2! non-null assertions are still
present in the single-table inheritance test, so this refactor doesn’t actually eliminate the
non-null-assertion pattern there. Since findOneOrFail rejects when the entity is missing, these
! assertions are redundant and likely continue triggering the same static-analysis feedback this
PR targets.
The test uses findOneOrFail to load loadedPerson1/loadedPerson2, but still applies non-null
assertions when casting. In the codebase, EntityManager.findOneOrFail explicitly rejects when the
result is null, meaning the promise only resolves with a non-null entity, so the non-null
assertions are unnecessary.
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
## Issue description
In `single-table/basic-functionality.test.ts`, `loadedPerson1` and `loadedPerson2` are loaded via `findOneOrFail` but are still cast using `loadedPerson1! as Student` / `loadedPerson2! as Employee`. Because `findOneOrFail` already guarantees non-null (or throws), these `!` operators are redundant and likely keep the Sonar warning this PR aims to remove.
### Issue Context
`EntityManager.findOneOrFail` rejects when the fetched value is `null`, so the variable can be treated as non-null without an assertion.
### Fix Focus Areas
- test/functional/table-inheritance/single-table/basic-functionality/basic-functionality.test.ts[427-457]
- src/entity-manager/EntityManager.ts[1376-1390]
### Suggested change
Replace:
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
4. loadedfeature not camelCase ☑ 📘⚙
Description
The updated test code uses identifiers like loadedfeature/updatedfeature, which deviate from the
project’s typical camelCase naming (e.g., loadedFeature). This reduces readability and violates
the naming-conventions requirement.
PR Compliance ID 1 requires consistent naming conventions for identifiers. The changed lines include
loadedfeature (and similarly updatedfeature later in the file), which is inconsistent with
standard camelCase naming.
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
## Issue description
The identifiers `loadedfeature` and `updatedfeature` do not follow camelCase naming (expected `loadedFeature` / `updatedFeature`).
## Issue Context
This PR touches these lines while refactoring `findOneBy()` to `findOneByOrFail()`, so it’s a good opportunity to align the variable names with project conventions.
## Fix Focus Areas
- test/core/column-kinds/geometry-column/geometry-column.test.ts[35-76]
- test/core/column-kinds/geometry-column/geometry-column.test.ts[93-128]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
ⓘ The new review experience is currently in Beta. Learn more
Is there some original variants left in the tests? Do we test them separately actually somewhere? Like, does the fail varian use the original variant under the hood? If so, we need to assert that.
Is there some original variants left in the tests? Do we test them separately actually somewhere? Like, does the fail varian use the original variant under the hood? If so, we need to assert that.
yes, do we have a test that checks correlation between fineOneOrFail and fineOne? (or, preferably, a spy with call count)
I don’t think we do, at least not by quick search. This would be a candidate for unit test, however it is hard to imagine this would be implemented in any other way then calling findX and checking results... @alumni do you think it’s worth to add this extra unit test for all OrFail methods?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replace
findOneBy(),findOne(), andgetOne()with theirOrFailvariants across the test suite, addressing the most common SonarQube feedback — unnecessary non-null assertions. Related to #12255.The
OrFailmethods throwEntityNotFoundErrorinstead of returningnull, which:!type assertions on every property accessexpect(x).to.not.be.nullchecks (the OrFail method already guarantees non-null)Also simplifies
findOneOrFail({ where: {...} })tofindOneByOrFail({...})andfindOne({ where: {...} })tofindOneBy({...})where no other options (relations, select, order) are used.Tests that intentionally verify null results (e.g., after transaction rollbacks or inner joins with no matches) are left unchanged with
findOne()/getOne().Also fixes
loadedfeature→loadedFeaturecamelCase naming in geometry column tests and replaces remaining non-null assertions on nullable properties with optional chaining.