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

Add saveCacheV2 tests #1879

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add saveCacheV2 tests #1879

wants to merge 3 commits into from

Conversation

johnsudol
Copy link
Contributor

@johnsudol johnsudol commented Nov 26, 2024

Fixes #1879

  • Similar to Add restoreCacheV2 tests #1876, this PR adds unit tests for the saveCacheV2
  • also updates the return type of uploadCacheFile and includes a debug message with the status returned on upload

@johnsudol johnsudol self-assigned this Nov 26, 2024
@johnsudol johnsudol requested a review from a team as a code owner November 26, 2024 00:47

test('save with missing input should fail', async () => {
const paths: string[] = []
const primaryKey = 'Linux-node-bb828da54c148048dd17899ba9fda624811cfb43'
Copy link
Member

Choose a reason for hiding this comment

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

nit: to be consistent with the restoreCacheV2 tests, how about we name this key?

})

test('save with large cache outputs should fail using', async () => {
const filePath = 'node_modules'
Copy link
Member

Choose a reason for hiding this comment

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

nit: also here, maybe it's better to use paths instead?

const paths = ['node_modules']

Comment on lines +130 to +134
expect(createCacheEntryMock).toHaveBeenCalledTimes(1)
expect(createCacheEntryMock).toHaveBeenCalledWith({
key: primaryKey,
version: cacheVersion
})
Copy link
Member

@Link- Link- Nov 26, 2024

Choose a reason for hiding this comment

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

Thinking out loud: are both of these necessary? Wouldn't one be sufficient?

Comment on lines +99 to +101
const createCacheEntryMock = jest
.spyOn(CacheServiceClientJSON.prototype, 'CreateCacheEntry')
.mockReturnValue(Promise.resolve({ok: false, signedUploadUrl: ''}))
Copy link
Member

Choose a reason for hiding this comment

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

As I'm reading the test, I'm thinking whether it might be better to shuffle the code, try to call CreateCacheEntry earlier and only create the archive if this call succeeds. Right now it seems we're doing a lot of work on the runner (creating the archive first) before checking whether we can create the cache entry

const filePath = 'node_modules'
const primaryKey = 'Linux-node-bb828da54c148048dd17899ba9fda624811cfb43'
const logWarningMock = jest.spyOn(core, 'warning')
const signedUploadURL = 'https://signed-upload-url.com'
Copy link
Member

Choose a reason for hiding this comment

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

nit: do we want to use the same url?

const signedDownloadUrl = 'https://blob-storage.local?signed=true'

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.

2 participants