scorecard example for backend tests#3414
Conversation
|
This pull request adds a new top-level directory under |
|
🤖 Finished Review · ✅ Success · Started 10:27 AM UTC · Completed 10:35 AM UTC |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3414 +/- ##
==========================================
+ Coverage 50.28% 50.30% +0.02%
==========================================
Files 2260 2260
Lines 85603 85603
Branches 24211 24203 -8
==========================================
+ Hits 43044 43064 +20
+ Misses 42058 42038 -20
Partials 501 501
*This pull request uses carry forward flags. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
ReviewFindingsLow
Info
Previous runReviewFindingsLow
Info
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| const BASE_CONFIG = { | ||
| backend: { | ||
| database: { | ||
| client: 'better-sqlite3', |
There was a problem hiding this comment.
[low] test-integrity
The afterAll hook calls server.close() without awaiting or handling its callback. http.Server.close() does not return a promise, so connections may still be draining when Jest proceeds. This can cause open handle warnings or flaky teardown. The same pattern appears at line 155.
| '/api/scorecard/metrics?metricIds=sonar.quality&datasource=github', | ||
| ); | ||
|
|
||
| expect(res.status).toBe(400); |
There was a problem hiding this comment.
[low] test-adequacy
The KPI config test defines type statusGrouped and asserts aggregationType statusGrouped, but this is also the default value. The assertion passes regardless of whether the config-driven path or the fallback is taken. Testing with a non-default type would provide stronger assurance.
fullsend-ai-review
Bot
added
the
ready-for-merge
christoph-jerolimov
left a comment
christoph-jerolimov
left a comment
There was a problem hiding this comment.
Awesome @teknaS47 👍
Small things:
Signed-off-by: Sanket Saikia <sanketsaikia13@gmail.com>
|
|
🤖 Finished Review · ✅ Success · Started 2:28 PM UTC · Completed 2:40 PM UTC |
| const res = await request(server).get( | ||
| '/api/scorecard/aggregations/non.existent/metadata', | ||
| ); | ||
|
|
There was a problem hiding this comment.
[low] test-inadequate
The entity-level metric tests only assert that the response is an empty array. These tests exercise route plumbing but never verify that actual metric results are returned correctly.
| it('returns all registered metrics', async () => { | ||
| const res = await request(server).get('/api/scorecard/metrics'); | ||
|
|
||
| expect(res.status).toBe(200); |
There was a problem hiding this comment.
[low] naming-convention
The test file uses res for supertest responses whereas existing router.test.ts uses response or result.
Suggested fix: Rename res to response or result.
|
|
||
| expect(res.status).toBe(200); | ||
| expect(res.body.metrics).toHaveLength(3); | ||
|
|
There was a problem hiding this comment.
[low] api-shape-pattern
Uses inline type annotations (m: { id: string }) => m.id instead of importing the domain Metric type as done in router.test.ts.
Suggested fix: Import Metric from @red-hat-developer-hub/backstage-plugin-scorecard-common and use (m: Metric) => m.id.
|
|
||
| expect(res.status).toBe(401); | ||
| }); | ||
|
|
There was a problem hiding this comment.
[info] test-inadequate
The KPI-config describe block does not pass any entities to startScorecardBackend. Currently only metadata tests exist so nothing breaks.
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
There was a problem hiding this comment.
[info] code-organization
Blank line between the license header and first import. Other test files have no blank line there.
fullsend-ai-review
Bot
added
ready-for-merge
2 participants
Hey, I just made a Pull Request!
RHIDP-14185
✔️ Checklist