Skip to content
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

increase coverage for setup app #12

Merged
merged 13 commits into from
Dec 19, 2024
Merged

increase coverage for setup app #12

merged 13 commits into from
Dec 19, 2024

Conversation

kswanson33
Copy link
Collaborator

No description provided.

@kswanson33 kswanson33 marked this pull request as draft December 13, 2024 16:21
@kswanson33 kswanson33 force-pushed the SHRMP-49/test-run-studies branch from a12b9cf to b7c1b55 Compare December 16, 2024 17:01
@kswanson33 kswanson33 marked this pull request as ready for review December 16, 2024 17:01
@kswanson33 kswanson33 force-pushed the SHRMP-49/test-run-studies branch from b7c1b55 to 202a491 Compare December 16, 2024 17:02
@kswanson33 kswanson33 requested a review from a team December 16, 2024 17:46
@kswanson33 kswanson33 force-pushed the SHRMP-49/test-run-studies branch from 72830ef to 1b7e5af Compare December 16, 2024 21:27
Copy link
Contributor

@sparksam sparksam left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Copy link

@philschatz philschatz left a comment

Choose a reason for hiding this comment

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

Just a couple suggestions

import jwt from 'jsonwebtoken'

beforeEach(() => {
process.env.MANAGEMENT_APP_PRIVATE_KEY = 'mockprivatekeyvalue'

Choose a reason for hiding this comment

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

Could these be set in the jest.env section in package.json?

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 @philschatz i think you left a similar comment on my test about using the jest setupFile feature, but i couldn't find a similar feature in vitest which we're using (i think referencing jest makes things a little more confusing since its not totally 1-1 with feature names or parity), i ended up taking this same approach for setting up/tearing down env variables here

i figure whatever we do in one place we should do in the other too

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks buddies 🙏

src/lib/utils.ts Outdated
}

// returns given value with type certainty, or silently exchanges value if null or undefined
export const ensureValueWithExchange = <T>(value: T, exchange: NonNullable<T>): NonNullable<T> => {

Choose a reason for hiding this comment

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

Would Or work?

src/lib/utils.ts Outdated
if (runId !== undefined && !managementAppRunIds.includes(runId) && item.ResourceARN !== undefined) {
orphanTaskDefinitions.push(item.ResourceARN)
}
})

return orphanTaskDefinitions
}

// returns given value with type certainty, or errors if value is null or undefined
export const ensureValueWithError = <T>(value: T, message?: string): NonNullable<T> => {

Choose a reason for hiding this comment

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

Suggested change
export const ensureValueWithError = <T>(value: T, message?: string): NonNullable<T> => {
export const ensureValueWithError = <T>(value: T | undefined | null, message?: string): T => {

Maybe easier to read... fewer negatives?

const baseTaskDefinition = process.env.BASE_TASK_DEFINITION_FAMILY || ''
const subnets = process.env.VPC_SUBNETS || ''
const securityGroup = process.env.SECURITY_GROUP || ''
const cluster = ensureValueWithExchange(process.env.ECS_CLUSTER, '')

Choose a reason for hiding this comment

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

Switch most (all?) ensureValueWithExchange to be ensureValueWithError

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will address in SHRMP-51 (made a comment on the issue)

@@ -7,11 +7,10 @@
"dev": "next dev",
"build": "next build",
"start": "next start",
"lint": "next lint && prettier --check .",
"lint:fix": "next lint --fix && prettier --write .",
Copy link
Contributor

Choose a reason for hiding this comment

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

how come no more next lint? just curious

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

next lint gives an error when those frontend files are removed (pages) and (apps)

Copy link
Contributor

Choose a reason for hiding this comment

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

so jealous you guys get to remove all your frontend files 😆

@kswanson33 kswanson33 merged commit 0268740 into main Dec 19, 2024
1 check passed
@kswanson33 kswanson33 deleted the SHRMP-49/test-run-studies branch December 19, 2024 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants