Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
12 changes: 11 additions & 1 deletion packages/core/src/shared/extensionUtilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,17 @@ export function isSageMaker(appName: 'SMAI' | 'SMUS' = 'SMAI'): boolean {
}

export function isCn(): boolean {
return getComputeRegion()?.startsWith('cn') ?? false
try {
const region = getComputeRegion()
if (!region || region === 'notInitialized') {
getLogger().debug('isCn called before compute region initialized, defaulting to false')
return false
}
return region.startsWith('cn')
} catch (err) {
getLogger().error(`Error in isCn method: ${err}`)
return false
}
Comment on lines +204 to +207
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we instead be throwing the error here? We already handle the known case above by returning false when the region is uninitialized, but this can catch other errors that I'm not sure we should assume returning false for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, the case that throws error is handled already, and i didnt find any error cases that could result from the getComputeRegion method, hence it looks safe to not throw error back

}

/**
Expand Down
61 changes: 60 additions & 1 deletion packages/core/src/test/shared/extensionUtilities.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { AWSError } from 'aws-sdk'
import * as sinon from 'sinon'
import { DefaultEc2MetadataClient } from '../../shared/clients/ec2MetadataClient'
import * as vscode from 'vscode'
import { UserActivity, getComputeRegion, initializeComputeRegion } from '../../shared/extensionUtilities'
import { UserActivity, getComputeRegion, initializeComputeRegion, isCn } from '../../shared/extensionUtilities'
import { isDifferentVersion, setMostRecentVersion } from '../../shared/extensionUtilities'
import { InstanceIdentity } from '../../shared/clients/ec2MetadataClient'
import { extensionVersion } from '../../shared/vscode/env'
Expand Down Expand Up @@ -135,6 +135,65 @@ describe('initializeComputeRegion, getComputeRegion', async function () {
})
})

describe('isCn', function () {
let sandbox: sinon.SinonSandbox
const metadataService = new DefaultEc2MetadataClient()

beforeEach(function () {
sandbox = sinon.createSandbox()
})

afterEach(function () {
sandbox.restore()
})

it('returns false when compute region is not initialized', async function () {
// Reset the compute region to undefined first
const utils = require('../../shared/extensionUtilities')
Object.defineProperty(utils, 'computeRegion', {
value: undefined,
configurable: true,
})

const result = isCn()

assert.strictEqual(result, false, 'isCn() should return false when compute region is not initialized')
})

it('returns true for CN regions', async function () {
sandbox.stub(metadataService, 'getInstanceIdentity').resolves({ region: 'cn-north-1' })
await initializeComputeRegion(metadataService, false, true)

const result = isCn()

assert.strictEqual(result, true, 'isCn() should return true for China regions')
})

it('returns false for non-CN regions', async function () {
sandbox.stub(metadataService, 'getInstanceIdentity').resolves({ region: 'us-east-1' })
await initializeComputeRegion(metadataService, false, true)

const result = isCn()

assert.strictEqual(result, false, 'isCn() should return false for non-China regions')
})

it('returns false when an error occurs', async function () {
const utils = require('../../shared/extensionUtilities')

// Restore original getComputeRegion if it was stubbed
if (utils.getComputeRegion.restore) {
utils.getComputeRegion.restore()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this? I see sandbox.restore() is called in our afterEach, doesn't this already reset all stubs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, not needed necessarily. Added it initially for best practice, but since sandbox is restored in afterEach, underlying stubs should also get cleaned


sandbox.stub(utils, 'getComputeRegion').throws(new Error('Test error'))

const result = isCn()

assert.strictEqual(result, false, 'isCn() should return false when an error occurs')
})
})

describe('UserActivity', function () {
let count: number
let sandbox: sinon.SinonSandbox
Expand Down
Loading