-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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.
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
fix: implement new graphql fields for spec counts #25757
Changes from 24 commits
df76d1c
06ca95e
55abda9
97d560a
cd88393
5ae892f
89cbf00
75c030f
174e5f1
e502f21
97131cf
6ec7f87
c13a6b3
4b3a56b
945a66f
0e72f2e
9d520fb
0a727c3
0f2712e
6f53db7
94a33c3
b4a1a0f
e7a4059
83a95b9
762dc2d
97b2b43
ae96ae7
8c1e747
f138cc3
e6c08fa
46e79a5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ import debugLib from 'debug' | |
import { isEqual } from 'lodash' | ||
|
||
import type { DataContext } from '../DataContext' | ||
import type { CloudSpecStatus, Query, RelevantRun, CurrentProjectRelevantRunSpecs, CloudSpecRun, CloudRun } from '../gen/graphcache-config.gen' | ||
import type { Query, RelevantRun, CurrentProjectRelevantRunSpecs, CloudRun } from '../gen/graphcache-config.gen' | ||
import { Poller } from '../polling' | ||
import type { CloudRunStatus } from '@packages/graphql/src/gen/cloud-source-types.gen' | ||
|
||
|
@@ -15,6 +15,8 @@ const RELEVANT_RUN_SPEC_OPERATION_DOC = gql` | |
id | ||
runNumber | ||
status | ||
completedInstanceCount | ||
totalInstanceCount | ||
specs { | ||
id | ||
status | ||
|
@@ -55,8 +57,6 @@ export const SPECS_EMPTY_RETURN: RunSpecReturn = { | |
statuses: {}, | ||
} | ||
|
||
const INCOMPLETE_STATUSES: CloudSpecStatus[] = ['RUNNING', 'UNCLAIMED'] | ||
|
||
export type RunSpecReturn = { | ||
runSpecs: CurrentProjectRelevantRunSpecs | ||
statuses: { | ||
|
@@ -86,23 +86,9 @@ export class RelevantRunSpecsDataSource { | |
return this.#cached.runSpecs | ||
} | ||
|
||
#calculateSpecMetadata (specs: CloudSpecRun[]) { | ||
//mimic logic in Cloud to sum up the count of groups per spec to give the total spec counts | ||
const countGroupsForSpec = (specs: CloudSpecRun[]) => { | ||
return specs.map((spec) => spec.groupIds?.length || 0).reduce((acc, curr) => acc += curr, 0) | ||
} | ||
|
||
return { | ||
totalSpecs: countGroupsForSpec(specs), | ||
completedSpecs: countGroupsForSpec(specs.filter((spec) => !INCOMPLETE_STATUSES.includes(spec.status || 'UNCLAIMED'))), | ||
} | ||
} | ||
|
||
/** | ||
* Pulls runs from the current Cypress Cloud account and determines which runs are considered: | ||
* - "current" the most recent completed run, or if not found, the most recent running run | ||
* - "next" the most recent running run if a completed run is found | ||
* @param shas list of Git commit shas to query the Cloud with for matching runs | ||
* Pulls the specs that match the relevant run. | ||
* @param runs - the current and (optionally) next relevant run | ||
*/ | ||
async getRelevantRunSpecs (runs: RelevantRun): Promise<RunSpecReturn> { | ||
const projectSlug = await this.ctx.project.projectId() | ||
|
@@ -153,22 +139,38 @@ export class RelevantRunSpecsDataSource { | |
statuses: {}, | ||
} | ||
|
||
if (cloudProject.current && cloudProject.current.runNumber && cloudProject.current.status) { | ||
const { current, next } = cloudProject | ||
|
||
if ( | ||
current | ||
&& current.runNumber | ||
&& current.status | ||
&& typeof current.totalInstanceCount === 'number' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool, sure 👍 |
||
&& typeof current.completedInstanceCount === 'number' | ||
) { | ||
runSpecsToReturn.runSpecs.current = { | ||
...this.#calculateSpecMetadata(cloudProject.current.specs || []), | ||
runNumber: cloudProject.current.runNumber, | ||
totalSpecs: current.totalInstanceCount, | ||
completedSpecs: current.completedInstanceCount, | ||
runNumber: current.runNumber, | ||
} | ||
|
||
runSpecsToReturn.statuses.current = cloudProject.current.status | ||
runSpecsToReturn.statuses.current = current.status | ||
} | ||
|
||
if (cloudProject.next && cloudProject.next.runNumber && cloudProject.next.status) { | ||
if ( | ||
next | ||
&& next.runNumber | ||
&& next.status | ||
&& typeof next.totalInstanceCount === 'number' | ||
&& typeof next.completedInstanceCount === 'number' | ||
) { | ||
runSpecsToReturn.runSpecs.next = { | ||
...this.#calculateSpecMetadata(cloudProject.next.specs || []), | ||
runNumber: cloudProject.next.runNumber, | ||
totalSpecs: next.totalInstanceCount, | ||
completedSpecs: next.completedInstanceCount, | ||
runNumber: next.runNumber, | ||
} | ||
|
||
runSpecsToReturn.statuses.next = cloudProject.next.status | ||
runSpecsToReturn.statuses.next = next.status | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This block is pretty much a duplicate of the one above, worth extracting to a fn? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was marginal on whether to do this or not, and we are about to be right back in here in the next sprint or two doing further modifications. So seems slightly easier on all sides to leave this logic repeated, make the diff super simple, and the shortly we'll combine them in a way that meets the needs we are about to have. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I went ahead and made a helper: |
||
|
||
return runSpecsToReturn | ||
|
@@ -208,7 +210,9 @@ export class RelevantRunSpecsDataSource { | |
debug('Run statuses changed') | ||
const projectSlug = await this.ctx.project.projectId() | ||
|
||
if (projectSlug) { | ||
const wasWatchingCurrentProject = this.#cached.statuses.current === 'RUNNING' | ||
|
||
if (projectSlug && wasWatchingCurrentProject) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the change that avoids a flash of the full page loading skeleton when a Ideally we would have an end-to-end test for this, I'm working on one that can form part of a separate PR if this merges first. It's also the kind of test that will be much easier to write if we have improvements coming in the test infra soon. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. It's how the code was in the video I put in the PR description where it's having the intended effect, let me take a look at why exactly that's the case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated to move this earlier in the flow, everything seems good anyway. |
||
debug(`Invalidate cloudProjectBySlug ${projectSlug}`) | ||
await this.ctx.cloud.invalidate('Query', 'cloudProjectBySlug', { slug: projectSlug }) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
import { expect } from 'chai' | ||
import sinon from 'sinon' | ||
|
||
import { DataContext } from '../../../src' | ||
import { createTestDataContext } from '../helper' | ||
import { RelevantRunSpecsDataSource, SPECS_EMPTY_RETURN } from '../../../src/sources' | ||
import { FAKE_PROJECT_ONE_RUNNING_RUN_ONE_COMPLETED_THREE_SPECS, FAKE_PROJECT_ONE_RUNNING_RUN_ONE_SPEC } from './fixtures/graphqlFixtures' | ||
|
||
describe('RelevantRunsDataSource', () => { | ||
marktnoonan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let ctx: DataContext | ||
let dataSource: RelevantRunSpecsDataSource | ||
|
||
beforeEach(() => { | ||
ctx = createTestDataContext('open') | ||
dataSource = new RelevantRunSpecsDataSource(ctx) | ||
sinon.stub(ctx.project, 'projectId').resolves('test123') | ||
}) | ||
|
||
describe('getRelevantRunSpecs()', () => { | ||
it('returns no specs or statuses when no specs found for run', async () => { | ||
const result = await dataSource.getRelevantRunSpecs({ current: 11111, next: 22222, commitsAhead: 0 }) | ||
|
||
expect(result).to.eql(SPECS_EMPTY_RETURN) | ||
}) | ||
|
||
it('returns expected specs and statuses when one run is found', async () => { | ||
sinon.stub(ctx.cloud, 'executeRemoteGraphQL').resolves(FAKE_PROJECT_ONE_RUNNING_RUN_ONE_SPEC) | ||
|
||
const result = await dataSource.getRelevantRunSpecs({ current: 1, next: null, commitsAhead: 0 }) | ||
|
||
expect(result).to.eql({ | ||
runSpecs: { | ||
current: { | ||
runNumber: 1, | ||
completedSpecs: 1, | ||
totalSpecs: 1, | ||
}, | ||
}, | ||
statuses: { current: 'RUNNING' }, | ||
}) | ||
}) | ||
|
||
it('returns expected specs and statuses when one run is completed and one is running', async () => { | ||
sinon.stub(ctx.cloud, 'executeRemoteGraphQL').resolves(FAKE_PROJECT_ONE_RUNNING_RUN_ONE_COMPLETED_THREE_SPECS) | ||
|
||
const result = await dataSource.getRelevantRunSpecs({ current: 1, next: null, commitsAhead: 0 }) | ||
|
||
expect(result).to.eql({ | ||
runSpecs: { | ||
current: { | ||
runNumber: 1, | ||
completedSpecs: 3, | ||
totalSpecs: 3, | ||
}, | ||
next: { | ||
runNumber: 2, | ||
completedSpecs: 0, | ||
totalSpecs: 3, | ||
}, | ||
}, | ||
statuses: { | ||
current: 'PASSED', | ||
next: 'RUNNING', | ||
}, | ||
}) | ||
}) | ||
}) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of modifying this on the fly, should we use the fixtures you added such as
FAKE_PROJECT_MULTIPLE_COMPLETED
?I find the modified on the fly ones kind of hard to read - you need to have a mental modal of exactly what you are modifying in the first place. How about you?